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

Apply repo mapping to cc_shared_library's exports_filter #21622

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Mar 8, 2024

This is required for exports_filter to match any external repositories with Bzlmod enabled.

native.package_relative_label is used to convert the user-specified filter strings, which are always valid labels, to canonical label literals that are then passed to the actual cc_shared_library rule.

Fixes #21872

@fmeum fmeum requested review from Wyverald and comius March 8, 2024 18:35
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Mar 8, 2024

"""Implementation of cc_shared_library's macro wrapper"""

def cc_shared_library(**kwargs):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This being a macro would require migrating the attribute docs to a function comment.

@comius What do you think of allowing native.package_relative_label in initializers instead, perhaps by providing a suitably trimmed down BazelStarlarkContext to the thread running the initializer? I understand why you don't want native.existing_rules to be run in one, but label conversion will be a frequent ask with Bzlmod.

Copy link
Contributor

Choose a reason for hiding this comment

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

Initializers already lift strings of regular labels. Maybe the solution is to make exports_filter a label?

Should the lifting really be package relative? That's only the property of output labels. Otherwise labels are lifted call-site relative. Not much difference at the moment. But if you had a string label in an initializer, that would need to work relative to the initializer, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Initializers already lift strings of regular labels. Maybe the solution is to make exports_filter a label?

Could you elaborate on that? My understanding is that making exports_filter a label_list would result in dependency edges on all entries of the list, which would result in errors since the __pkg__ pseudo targets don't exist.

Should the lifting really be package relative? That's only the property of output labels. Otherwise labels are lifted call-site relative. Not much difference at the moment. But if you had a string label in an initializer, that would need to work relative to the initializer, right?

I'm aware of three different "lifting" mechanisms in macros:

  • Strings passed to attr.labels are resolved relative to the BUILD file call site.
  • The Label constructor resolves relative to its own .bzl call site.
  • native.package_relative_label resolves relative to the BUILD file call site.

I wouldn't expect initializers to behave differently here. But at the moment native.package_relative_label isn't available in initializers, so I don't know how to turn user-supplied strings into labels without creating a dependency edge.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t want to expose such functions to initializers, because they open space for crafting weird conversions and bugs with that.

A new type like attr.nodep_label_list would be the right thing to do. We have it natively.

But I’m also wondering, what’s going on in cc_shared_library, to make a usecase like this. In all the rules we touched, label comparison was a bad idea and we removed it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@oquenchil can explain that better than me. Having an attribute control the transitive closure of a top-level transitive target does seem like a reasonable if rare use case to me.

Given that this is essentially a bug fix for a regression caused by --enable_bzlmod, what do you think of using a macro for now until something like attr.nodep_label_list becomes available?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think it's better to expose package_relative_label to initializers and use them.

Without initializers also the docs on bazel.build would break.

Copy link
Collaborator Author

@fmeum fmeum Mar 20, 2024

Choose a reason for hiding this comment

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

I implemented this in #21740. I also allowed the other non-target related functions as they seemed equally safe and useful to allow in initializers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am now using an initializer, PTAL.

@fmeum
Copy link
Collaborator Author

fmeum commented Mar 8, 2024

@fmeum
Copy link
Collaborator Author

fmeum commented Mar 20, 2024

Switching to draft as I will update it after #21740 has been merged.

@fmeum fmeum marked this pull request as draft March 20, 2024 11:11
copybara-service bot pushed a commit that referenced this pull request May 7, 2024
Work towards #21622

RELNOTES: `native.package_relative_label` can now be used in rule initializers.

Closes #21740.

PiperOrigin-RevId: 631546734
Change-Id: Id6eb237f0f79195b678bb954deaef071e1c45ab1
This is required for `exports_filter` to match any external repositories with Bzlmod enabled.

`native.package_relative_label` is used to convert the user-specified filter strings, which are always valid labels, to canonical label literals that are then passed to the actual `cc_shared_library` rule.
@fmeum fmeum force-pushed the cc-shared-library-apparent branch from bad9800 to 4be413d Compare May 8, 2024 07:03
@fmeum
Copy link
Collaborator Author

fmeum commented May 8, 2024

@bazel-io fork 7.2.0

@fmeum fmeum marked this pull request as ready for review May 8, 2024 07:03
@comius comius added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels May 8, 2024
fmeum added a commit to fmeum/bazel that referenced this pull request May 10, 2024
Work towards bazelbuild#21622

RELNOTES: `native.package_relative_label` can now be used in rule initializers.

Closes bazelbuild#21740.

PiperOrigin-RevId: 631546734
Change-Id: Id6eb237f0f79195b678bb954deaef071e1c45ab1
@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 May 10, 2024
Wyverald pushed a commit that referenced this pull request May 11, 2024
This is required for `exports_filter` to match any external repositories with Bzlmod enabled.

`native.package_relative_label` is used to convert the user-specified filter strings, which are always valid labels, to canonical label literals that are then passed to the actual `cc_shared_library` rule.

Fixes #21872

Closes #21622.

PiperOrigin-RevId: 632624776
Change-Id: I2f80563d8c434b2726f4facb0551b316b2cd2e1c
github-merge-queue bot pushed a commit that referenced this pull request May 11, 2024
…22328)

This is required for `exports_filter` to match any external repositories
with Bzlmod enabled.

`native.package_relative_label` is used to convert the user-specified
filter strings, which are always valid labels, to canonical label
literals that are then passed to the actual `cc_shared_library` rule.

Fixes #21872

Closes #21622.

PiperOrigin-RevId: 632624776
Change-Id: I2f80563d8c434b2726f4facb0551b316b2cd2e1c

Co-authored-by: Fabian Meumertzheim <[email protected]>
@fmeum fmeum deleted the cc-shared-library-apparent branch May 11, 2024 19:37
Kila2 pushed a commit to Kila2/bazel that referenced this pull request May 13, 2024
Work towards bazelbuild#21622

RELNOTES: `native.package_relative_label` can now be used in rule initializers.

Closes bazelbuild#21740.

PiperOrigin-RevId: 631546734
Change-Id: Id6eb237f0f79195b678bb954deaef071e1c45ab1
Kila2 pushed a commit to Kila2/bazel that referenced this pull request May 13, 2024
This is required for `exports_filter` to match any external repositories with Bzlmod enabled.

`native.package_relative_label` is used to convert the user-specified filter strings, which are always valid labels, to canonical label literals that are then passed to the actual `cc_shared_library` rule.

Fixes bazelbuild#21872

Closes bazelbuild#21622.

PiperOrigin-RevId: 632624776
Change-Id: I2f80563d8c434b2726f4facb0551b316b2cd2e1c
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.

cc_shared_library.exports_filter doesn't support apparent repo names
4 participants