Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --incompatible_disable_objc_library_transition #19256

Conversation

keith
Copy link
Member

@keith keith commented Aug 15, 2023

This is a migration for #16870

Users who rely on the current behavior should instead wrap their library in a target from rules_apple.

Fixes #19204

@keith keith requested a review from lberki as a code owner August 15, 2023 17:44
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Rules-CPP Issues for C++ rules labels Aug 15, 2023
@cpsauer
Copy link
Contributor

cpsauer commented Aug 16, 2023

Can confirm that, when flipped, this leads to clean builds both under rules_apple and under cc_binary in our teams repo's fairly large set of cc <-> objc interoperability cases, fixing #19204. Thanks, Keith!
(For any others: Was testing by splicing in Bazel's //src and passing --experimental_builtins_bzl_path="%workspace%")

To ease the migration, might you at some point also want some of the test changes from #19236, easing assumptions on under-configured sets of internal flags?

@buildbreaker2021 buildbreaker2021 added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Aug 16, 2023
Copy link
Contributor

@googlewalt googlewalt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test for the flag?

@keith
Copy link
Member Author

keith commented Aug 18, 2023

FYI @comius since it looks like you're working in this area too with 03b8616

@comius
Copy link
Contributor

comius commented Aug 22, 2023

Nice, thanks!

I was also looking into removing this transition. It needs some depo fixes, but not a lot of them. So once this is merges, I can look into flipping the default value.

@keith keith force-pushed the ks/add-incompatible_disable_objc_library_transition branch from 31fe00e to 4497176 Compare August 22, 2023 16:15
@keith
Copy link
Member Author

keith commented Aug 22, 2023

Added a quick test!

This is a migration for bazelbuild#16870

Users who rely on the current behavior should instead wrap their library
in a target from [rules_apple](https://github.com/bazelbuild/rules_apple).
@keith keith force-pushed the ks/add-incompatible_disable_objc_library_transition branch from 4497176 to 67ab1b6 Compare August 25, 2023 16:03
@erneestoc
Copy link

I hit a related issue yesterday! Thank you 🙏🏻

@brentleyjones
Copy link
Contributor

@bazelbuild/triage Is this being manually imported or does it need the label?

@iancha1992
Copy link
Member

@brentleyjones @comius @buildbreaker2021 if this needs to be merged to the master, then please add the label, "awaiting-PR-merge". We'll handle it afterwards. Thanks!

@googlewalt
Copy link
Contributor

Unfortunately, I don't have the ability to add labels.

@brentleyjones brentleyjones added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-user-response Awaiting a response from the author labels Aug 29, 2023
@brentleyjones
Copy link
Contributor

Based on your comment I just changed them. Thanks @googlewalt!

@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 30, 2023
@brentleyjones
Copy link
Contributor

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Aug 31, 2023
@iancha1992
Copy link
Member

@bazel-io fork 6.4.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Aug 31, 2023
@iancha1992
Copy link
Member

iancha1992 commented Aug 31, 2023

@comius @cpsauer @keith @googlewalt @buildbreaker2021 @brentleyjones
We've tried to cherry-pick this to release-6.4.0 branch, but looks like there are some conflicts.

  1. src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java
public static final String EXPERIMENTAL_GET_FIXED_CONFIGURED_ACTION_ENV =
      "-experimental_get_fixed_configured_action_env";

should be removed from the release-6.4.0 branch before cherry-picking

  1. src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java
.setBool(
                EXPERIMENTAL_GET_FIXED_CONFIGURED_ACTION_ENV,
                experimentalGetFixedConfiguredEnvironment)

before the cherry-picking, the release branch should have above removed and instead have below:

.setBool(
                INCOMPATIBLE_MERGE_FIXED_AND_DEFAULT_SHELL_ENV,
                incompatibleMergeFixedAndDefaultShellEnv)
            .setBool(
                INCOMPATIBLE_OBJC_PROVIDER_REMOVE_LINKING_INFO,
                incompatibleObjcProviderRemoveLinkingInfo)
  1. src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
    public void checkExperimentalStarlarkCcImport function should be removed from the release-6.4.0 branch before cherry-picking

  2. src/main/java/com/google/devtools/build/lib/starlarkbuildapi/cpp/CcModuleApi.java

@StarlarkMethod(
      name = "check_experimental_starlark_cc_import",
      doc = "DO NOT USE. This is to guard use of cc_import.bzl",
      documented = false,
      parameters = {
        @Param(
            name = "actions",
            positional = false,
            named = true,
            doc = "<code>actions</code> object."),
      })

should be removed from the release branch before cherry-picking

  1. src/main/starlark/builtins_bzl/common/cc/cc_common.bzl
    The file isn't in the release branch. It should be added to the release branch before cherry-picking.

load(":common/cc/cc_info.bzl", "CcInfo")

,

load("@_builtins//:common/objc/semantics.bzl", "semantics")

,

load("@_builtins//:common/objc/objc_common.bzl", "extensions", "objc_common")

should be added to the release branch before cherry-picking

cc: @bazelbuild/triage

@keith keith deleted the ks/add-incompatible_disable_objc_library_transition branch August 31, 2023 21:06
@keith
Copy link
Member Author

keith commented Sep 1, 2023

#19393

keith added a commit to keith/bazel that referenced this pull request Sep 1, 2023
This is a migration for bazelbuild#16870

Users who rely on the current behavior should instead wrap their library in a target from [rules_apple](https://github.com/bazelbuild/rules_apple).

Fixes bazelbuild#19204

Closes bazelbuild#19256.

PiperOrigin-RevId: 561253613
Change-Id: I1b1b24a08caa33e86881bfe376a525060c9887a9
(cherry picked from commit 0f34e76)
keith added a commit to keith/bazel that referenced this pull request Sep 6, 2023
This is a migration for bazelbuild#16870

Users who rely on the current behavior should instead wrap their library in a target from [rules_apple](https://github.com/bazelbuild/rules_apple).

Fixes bazelbuild#19204

Closes bazelbuild#19256.

PiperOrigin-RevId: 561253613
Change-Id: I1b1b24a08caa33e86881bfe376a525060c9887a9
(cherry picked from commit 0f34e76)
@iancha1992
Copy link
Member

The changes in this PR have been included in Bazel 6.4.0 RC1. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=last_rc.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Objc transition can swap architecture, leading to link failure
9 participants