-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Duplicate label detection doesn't allow commonality across disjoint selects #13785
Comments
What do you think is the right approach? I think the problem is that In other words, how do we know in the loading phase that the above paths are disjoint? |
I definitely don't know this area of code enough to have good ideas, but some uninformed thoughts:
|
I appreciate 1. I don't know the history behind that decision, if it's still relevant, or if it's always been too cautious. I think it's fair to revisit that assumption, and a little bit of code history spelunking would be the path to figure out next steps on that. |
Just to clarify, is this mostly a convenience issue over an expressiveness issue? (That is, in all cases, the associated selects could have been merged.) If convenience issue, do any of the functions in the selects skylib library help? |
Yes, this is mostly a convenience issue - as far as I can tell in our case we could always flatten the selects if needed. |
## What is the goal of this PR? We add the cucumber testing framework crate to our pinned rust dependencies. ## What are the changes implemented in this PR? While making this change, we encountered an issue in bazel (bazelbuild/bazel#13785) that made it throw an error because, in essence, libc was included in two out of three conditions, which was regarded as a dependency duplication. Cargo-raze is aware of this issue (google/cargo-raze#451) and has their own workaround in the works (google/cargo-raze#512) which is not being reviewed or merged due to reviewer bandwidth issues. As our own workaround, we separate the inclusion of libc into its own condition, overriding raze and pacifying bazel. This change is stored separately from the razed crates and reapplied when `library/crates/update.sh` is run.
## What is the goal of this PR? We add the cucumber testing framework crate to our pinned rust dependencies. ## What are the changes implemented in this PR? While making this change, we encountered an issue in bazel (bazelbuild/bazel#13785) that made it throw an error because, in essence, libc was included in two out of three conditions, which was regarded as a dependency duplication. Cargo-raze is aware of this issue (google/cargo-raze#451) and has their own workaround in the works (google/cargo-raze#512) which is not being reviewed or merged due to reviewer bandwidth issues. As our own workaround, we separate the inclusion of libc into its own condition, overriding raze and pacifying bazel. This change is stored separately from the razed crates and reapplied when `library/crates/update.sh` is run.
Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team ( |
Still valid :) |
Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale. |
Still still valid |
Example of bug:
This should be valid, as the selects which add
test.sh
tosrcs
are disjoint, but Bazel is overly strict in its implementation:bazel/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java
Lines 184 to 205 in c785f02
The text was updated successfully, but these errors were encountered: