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

Duplicate label detection doesn't allow commonality across disjoint selects #13785

Open
illicitonion opened this issue Aug 2, 2021 · 9 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request

Comments

@illicitonion
Copy link
Contributor

Example of bug:

filegroup(
    name = "fg",
    srcs = select({
        "@platforms//os:linux": ["test.sh"],
        "//conditions:default": [],
    }) + select({
        "@platforms//os:osx": ["test.sh"],
        "//conditions:default": [],
    }), 
)

This should be valid, as the selects which add test.sh to srcs are disjoint, but Bazel is overly strict in its implementation:

// Multiple selects concatenated together. It's expensive to iterate over every possible
// permutation of values, so instead check for duplicates within a single select branch while
// also collecting all labels for a cross-select duplicate check at the end. This is overly
// strict, since this counts values present in mutually exclusive select branches. We can
// presumably relax this if necessary, but doing so would incur some of the expense this code
// path avoids.
ImmutableSet.Builder<Label> duplicates = null;
List<Label> combinedLabels = new ArrayList<>(); // Labels that appear across all selectors.
for (Selector<List<Label>> selector : selectors) {
// Labels within a single selector. It's okay for there to be duplicates as long as
// they're in different selector paths (since only one path can actually get chosen).
Set<Label> selectorLabels = new LinkedHashSet<>();
for (List<Label> labelsInSelectorValue : selector.getEntries().values()) {
// Duplicates within a single select branch are not okay.
duplicates = addDuplicateLabels(duplicates, labelsInSelectorValue);
selectorLabels.addAll(labelsInSelectorValue);
}
combinedLabels.addAll(selectorLabels);
}
duplicates = addDuplicateLabels(duplicates, combinedLabels);
return duplicates == null ? ImmutableSet.of() : duplicates.build();

@aiuto aiuto added team-Configurability platforms, toolchains, cquery, select(), config transitions untriaged labels Aug 3, 2021
@gregestren
Copy link
Contributor

What do you think is the right approach?

I think the problem is that AggregatingAttributeMapper, which focuses on the loading phase, doesn't know which combinations are actually possible. Worst case it can consider the cartesian product of every select. But that's worse in every way: it would trigger on impossible combinations and have performance issues.

In other words, how do we know in the loading phase that the above paths are disjoint?

@illicitonion
Copy link
Contributor Author

I definitely don't know this area of code enough to have good ideas, but some uninformed thoughts:

  1. For some attributes (I'm specifically thinking srcs and deps), the behaviour we want isn't actually to be a List but is to be a Set - perhaps we could actually model them as LinkedHashSets and just ignore duplicates? I've always found it slightly weird that we constrain these attributes to be "lists with no duplicates" where really we treat them as sets.
  2. If we don't like treating them as sets perhaps we could defer the duplicate checking until we're looking at ConfiguredTargets? AFAIK (but again, ill-informed) there's nothing which happens between parsing raw Targets and configuring them that would find the two being inconsistent problematic, but I can believe there are some weird corners

@gregestren
Copy link
Contributor

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.

@katre katre added P3 We're not considering working on this, but happy to review a PR. (No assignee) type: feature request and removed untriaged labels Sep 8, 2021
@sdtwigg
Copy link
Contributor

sdtwigg commented Sep 8, 2021

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?

@illicitonion
Copy link
Contributor Author

Yes, this is mostly a convenience issue - as far as I can tell in our case we could always flatten the selects if needed.

dmitrii-ubskii added a commit to typedb/typedb-dependencies that referenced this issue Nov 23, 2022
## 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.
jamesreprise pushed a commit to jamesreprise/vaticle-dependencies that referenced this issue Dec 5, 2022
## 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.
@github-actions
Copy link

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 (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label May 25, 2023
@illicitonion
Copy link
Contributor Author

Still valid :)

@github-actions github-actions bot removed the stale Issues or PRs that are stale (no activity for 30 days) label May 26, 2023
Copy link

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.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Jul 30, 2024
@illicitonion
Copy link
Contributor Author

Still still valid

@github-actions github-actions bot removed the stale Issues or PRs that are stale (no activity for 30 days) label Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request
Projects
None yet
Development

No branches or pull requests

5 participants