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

incompatible_remap_main_repo: treat @main_repo//, @//, and // labels as equivalent. #7130

Closed
dkelmer opened this issue Jan 15, 2019 · 25 comments
Closed

Comments

@dkelmer
Copy link
Contributor

@dkelmer dkelmer commented Jan 15, 2019

All PRs related to this change have been submitted and the behavior will not continue to evolve so this feature should be promoted to non-experimental state

@dkelmer dkelmer changed the title Promote --experimental_remap_main_repo to not be experimental incompatible_remap_main_repo Jan 16, 2019
@dkelmer

This comment has been minimized.

Copy link
Contributor Author

@dkelmer dkelmer commented Jan 16, 2019

This flag changes the evaluation of labels where within a repository called "foo", the labels:
@foo//some/path:lib
//some/path:lib
will be treated as the same label. Right now they are treated as two different labels.

This incompatible change is fixing a bug but it is possible that people are relying on the broken behavior in unexpected ways. Chances are no one will need to do anything, but if you encounter issues, please respond on this thread.

bazel-io pushed a commit that referenced this issue Jan 17, 2019
This flag fixes a bug in bazel where within a repository called "foo", the labels @foo//some/path:bar.bzl and //some/path:bar.bzl were treated as different labels. It is unlikely that this flag will affect any projects but it is technically a breaking change and so it is guarded behind a flag.

Towards #7130

RELNOTES: None
PiperOrigin-RevId: 229813128
weixiao-huang added a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
This flag fixes a bug in bazel where within a repository called "foo", the labels @foo//some/path:bar.bzl and //some/path:bar.bzl were treated as different labels. It is unlikely that this flag will affect any projects but it is technically a breaking change and so it is guarded behind a flag.

Towards bazelbuild#7130

RELNOTES: None
PiperOrigin-RevId: 229813128
bazel-io pushed a commit that referenced this issue Feb 27, 2019
Related to #7130

RELNOTES: Set default value of --incompatible_remap_main_repo to true.
PiperOrigin-RevId: 235945449
bazel-io pushed a commit that referenced this issue Feb 28, 2019
*** Reason for rollback ***

Broke Bazel CI: #7577

*** Original change description ***

Set default value of --incompatible_remap_main_repo to true.

Related to #7130

RELNOTES: Set default value of --incompatible_remap_main_repo to true.
PiperOrigin-RevId: 236084411
@katre

This comment has been minimized.

Copy link
Member

@katre katre commented Mar 1, 2019

This issue was tagged as "breaking-change-0.24" but does not appear ready to be flipped in the 0.24.0 release. If this is incorrect please comment on that issue and discuss with me.

@dkelmer

This comment has been minimized.

Copy link
Contributor Author

@dkelmer dkelmer commented Mar 4, 2019

@katre I was fully intending to flip it this time but I accidentally set a reminder to flip the flag right before the 0.24 cut as opposed to right after the 0.23 cut. Surprise surprise, there were breakages and I now missed the deadline :(

So I'm now aiming for 0.25. Is there something I should update?

@katre

This comment has been minimized.

Copy link
Member

@katre katre commented Mar 4, 2019

No, the labels are accurate now.

@dkelmer

This comment has been minimized.

Copy link
Contributor Author

@dkelmer dkelmer commented Mar 4, 2019

Thanks!

@deeglaze

This comment has been minimized.

Copy link

@deeglaze deeglaze commented Mar 18, 2019

What is y'all's recommendation for a crosstool that depends on project X's sources to build project X, project Y (external dependency of X), and project Z (depends on X as an external dependency)?

I haven't found any way to make a single crosstool definition work for all three cases when --incompatible_remap_main_repo=true.

We get all 3 cases to work when the flag is false and our crosstool always references project X path via external/project_x_workspace/path
https://github.com/google/asylo/blob/master/asylo/distrib/sgx_x86_64/CROSSTOOL.tpl#L49

If I disregard project Z, then dropping external/project_x_workspace/ works fine. If I add both path and external/project_x_workspace/path to my crosstool, then #include_next breaks within project X and project Y, since both paths exist, and the first will go to the second instead of the system header I want.

I've since migrated the referenced CROSSTOOL to the skylark representation, and thought that I could possibly detect whether the crosstool is used by an external dependency or an internal dependency and add the appropriate paths, but this is both nasty and I don't know if it's possible.

@deeglaze

This comment has been minimized.

Copy link

@deeglaze deeglaze commented Mar 20, 2019

Would it be possible for Bazel to create a symlink external/main_repo_name to the main repo's source directory so that there is a canonical path to the workspace's sources both inside and outside the workspace? If the labels are the same, I think the path structures should resolve to the same place, rather than one no longer existing.

@hlopko

This comment has been minimized.

Copy link
Contributor

@hlopko hlopko commented Mar 29, 2019

@deeglaze Would it be possible to create a tiny repro repository that demonstrates all your requirements that I can play with? Right now I don't see what changes with --incompatible_remap_main_repo and my naive repro attempt is not successful.

With Crosstool in Starlark the next logical step would be to accept artifacts for tool paths. I'd like to play with it more and your input would be very helpful. Related discussion is happening over at #7746, feel free to join.

katre added a commit that referenced this issue Aug 23, 2019
By the way we propagate the information in which repository we are in,
we always have a repository name that is valid in the main repository;
however, it is not necessarily a canonical name. Our map of the repository
mappings, however, is keyed by canonical names. So, add the missing indirection
and first look up the canonical name for the repository name we're given
by looking it up in the mapping of the main repository (for which we know
its canonical name) and only then use that name as key.

Fixes a bug related to #7130.
Prerequisite to flip --incompatible_remap_main_repo.
Therefore, consider for Bazel 0.29.0 (#8572).

Change-Id: Icd426a18aaa406b614f4948a8122177463a72959
PiperOrigin-RevId: 265012106
@aiuto

This comment has been minimized.

Copy link
Contributor

@aiuto aiuto commented Sep 3, 2019

@dslomov So this is going to not flip for 1.0? I'll put in a last plea that we should flip it even though things break downstream. We know 1.0 will break many projects downstream. This is the chance to correct a very strange bit of behavior that is incredibly hard to work around.

@dslomov

This comment has been minimized.

Copy link
Contributor

@dslomov dslomov commented Sep 4, 2019

The problem is, @aehlig has discovered some issues with implementation of this (related to interactions between repository mappings and toolchain registration) that are hard to investigate and fix. it is not that downstream projects are broken, it is that the implementation is not fully ready. So, regrettably, we will have to keep this off.

@dhalperi

This comment has been minimized.

Copy link
Contributor

@dhalperi dhalperi commented Sep 4, 2019

@dslomov

This comment has been minimized.

Copy link
Contributor

@dslomov dslomov commented Sep 5, 2019

Look, I agree it is unfortunate. The question is, is it a 1.0 blocker? It is not a recent regression - this has always been this way, since very early Bazel releases, and people were able to work around this. If we can fix the underlying issues fast and be confident that the implementation of this fix is rock-solid, I would gladly wait. But it does not look that way at the moment.

bazel-io pushed a commit that referenced this issue Sep 20, 2019
Target patterns are to be read in the name space of the main
repository. This namespace, however, can also contain a mapping
of repository names, in particular that of the main repository
(#7130, #3155).

While there, also make an error message more informative. (This
message change is related as the modified evaluation order causes
another error to be found first in a test example.)

Change-Id: I022240e5c201d33e31f2a818f74a91af3b6f7b3d
PiperOrigin-RevId: 270240453
bazel-io pushed a commit that referenced this issue Sep 25, 2019
...by taking the applicable repository mapping into account, instead
of simply assuming it is empty.

Related to #7130, #3115.

Change-Id: I7f3274b7e238ec0a9ad00643b738fbb12b84a872
PiperOrigin-RevId: 271133642
@laurentlb

This comment has been minimized.

Copy link
Member

@laurentlb laurentlb commented Nov 15, 2019

@aehlig Did you flip the flag for Bazel 2.0? If so, please close this issue.

@aehlig

This comment has been minimized.

Copy link
Member

@aehlig aehlig commented Nov 18, 2019

@aehlig Did you flip the flag for Bazel 2.0? If so, please close this issue.

The flag was flipped in 81b57d6, but there are some discussions in #10246 about undoing the flag flip. Stll, at the moment, the flag is flipped.

@aehlig aehlig closed this Nov 18, 2019
bazel-io pushed a commit that referenced this issue Dec 19, 2019
Baseline: 807ed23

Cherry picks:

   + db0e32c:
     build.sh: Fix bug in build script for RC release
   + 85e84f7:
     Set --incompatible_prohibit_aapt1 default to true.
   + 84eae2f:
     Let shellzelisk fallback to bazel-real if it's the requested
     version.
   + d5ae460:
     Fix a typo in bazel.sh

Incompatible changes:

  - --incompatible_remap_main_repo is enabled by default. Therefore,
    both ways of addressing the main repository, by its name and by
    '@' are now considered referring to the same repository.
    see #7130
  - --incompatible_disallow_dict_lookup_unhashable_keys is enabled by
    default #9184
  - --incompatible_remove_native_maven_jar is now enabled by default
    and the flag removed. See #6799
  - --incompatible_prohibit_aapt1 is enabled by default.
    See #10000

Important changes:

  - --incompatible_proto_output_v2: proto v2 for aquery proto output
    formats, which reduces the output size compared to v1. Note that
    the messages' ids in v2 are in uint64 instead of string like in
    v1.
  - Adds --incompatible_remove_enabled_toolchain_types.
  - Package loading now consistently fails if package loading had a
    glob evaluation that encountered a symlink cycle or symlink
    infinite expansion. Previously, such package loading with such
    glob evaluations would fail only in some cases.
  - The --disk_cache flag can now also be used together
    with the gRPC remote cache.
  - An action's discover inputs runtime metrics is now categorized as
    parse time on the CriticalPathComponent.
  - Make the formatting example more like to the written text by
    adding an initial description.
  - An action's discover inputs runtime metrics is now categorized as
    parse time on the CriticalPathComponent.
  - Bazel's Debian package and the binary installer now include an
    improved wrapper that understands `<WORKSPACE>/.bazelversion`
    files and the `$USE_BAZEL_VERSION` environment variable. This is
    similar to what Bazelisk offers
    (https://github.com/bazelbuild/bazelisk#how-does-bazelisk-know-whi
    ch-bazel-version-to-run-and-where-to-get-it-from), except that it
    works offline and integrates with apt-get.
  - We are planning to deprecate the runfiles manifest files, which
    aren't safe in the presence of whitespace, and also unnecessarily
    require local CPU when remote execution is used. This release
    adds --experimental_skip_runfiles_manifests to disable the
    generation of the input manifests (rule.manifest files) in most
    cases. Note that this flag has no effect on Windows by default or
    if --experimental_enable_runfiles is explicitly set to false.

This release contains contributions from many people at Google, as well as aldersondrive, Benjamin Peterson, Bor Kae Hwang, David Ostrovsky, Jakob Buchgraber, Jin, John Millikin, Keith Smiley, Lauri Peltonen, nikola-sh, Peter Mounce, Tony Hsu.
meteorcloudy pushed a commit to meteorcloudy/bazel that referenced this issue Jan 10, 2020
Baseline: 807ed23

Cherry picks:

   + db0e32c:
     build.sh: Fix bug in build script for RC release
   + 85e84f7:
     Set --incompatible_prohibit_aapt1 default to true.
   + 84eae2f:
     Let shellzelisk fallback to bazel-real if it's the requested
     version.
   + d5ae460:
     Fix a typo in bazel.sh

Incompatible changes:

  - --incompatible_remap_main_repo is enabled by default. Therefore,
    both ways of addressing the main repository, by its name and by
    '@' are now considered referring to the same repository.
    see bazelbuild#7130
  - --incompatible_disallow_dict_lookup_unhashable_keys is enabled by
    default bazelbuild#9184
  - --incompatible_remove_native_maven_jar is now enabled by default
    and the flag removed. See bazelbuild#6799
  - --incompatible_prohibit_aapt1 is enabled by default.
    See bazelbuild#10000

Important changes:

  - --incompatible_proto_output_v2: proto v2 for aquery proto output
    formats, which reduces the output size compared to v1. Note that
    the messages' ids in v2 are in uint64 instead of string like in
    v1.
  - Adds --incompatible_remove_enabled_toolchain_types.
  - Package loading now consistently fails if package loading had a
    glob evaluation that encountered a symlink cycle or symlink
    infinite expansion. Previously, such package loading with such
    glob evaluations would fail only in some cases.
  - The --disk_cache flag can now also be used together
    with the gRPC remote cache.
  - An action's discover inputs runtime metrics is now categorized as
    parse time on the CriticalPathComponent.
  - Make the formatting example more like to the written text by
    adding an initial description.
  - An action's discover inputs runtime metrics is now categorized as
    parse time on the CriticalPathComponent.
  - Bazel's Debian package and the binary installer now include an
    improved wrapper that understands `<WORKSPACE>/.bazelversion`
    files and the `$USE_BAZEL_VERSION` environment variable. This is
    similar to what Bazelisk offers
    (https://github.com/bazelbuild/bazelisk#how-does-bazelisk-know-whi
    ch-bazel-version-to-run-and-where-to-get-it-from), except that it
    works offline and integrates with apt-get.
  - We are planning to deprecate the runfiles manifest files, which
    aren't safe in the presence of whitespace, and also unnecessarily
    require local CPU when remote execution is used. This release
    adds --experimental_skip_runfiles_manifests to disable the
    generation of the input manifests (rule.manifest files) in most
    cases. Note that this flag has no effect on Windows by default or
    if --experimental_enable_runfiles is explicitly set to false.

This release contains contributions from many people at Google, as well as aldersondrive, Benjamin Peterson, Bor Kae Hwang, David Ostrovsky, Jakob Buchgraber, Jin, John Millikin, Keith Smiley, Lauri Peltonen, nikola-sh, Peter Mounce, Tony Hsu.
bazel-io pushed a commit that referenced this issue Jan 15, 2020
#7130

RELNOTES: The flag --incompatible_remap_main_repo is removed.
PiperOrigin-RevId: 289822459
laurentlb added a commit to laurentlb/bazel-federation that referenced this issue Jan 15, 2020
It's on by default since Bazel 2.0, and the flag is now going away.

bazelbuild/bazel#7130
laurentlb added a commit to laurentlb/bazel-skylib that referenced this issue Jan 15, 2020
It's on by default since Bazel 2.0, and the flag is now going away.

bazelbuild/bazel#7130
laurentlb added a commit to laurentlb/envoy that referenced this issue Jan 15, 2020
It's on by default since Bazel 2.0, and the flag is now going away.

bazelbuild/bazel#7130
laurentlb added a commit to laurentlb/envoy that referenced this issue Jan 15, 2020
It's on by default since Bazel 2.0, and the flag is now going away.

bazelbuild/bazel#7130
Signed-off-by: Laurent Le Brun <laurentlb@gmail.com>
mattklein123 added a commit to envoyproxy/envoy that referenced this issue Jan 15, 2020
It's on by default since Bazel 2.0, and the flag is now going away.

bazelbuild/bazel#7130
Signed-off-by: Laurent Le Brun <laurentlb@gmail.com>
laurentlb added a commit to bazelbuild/bazel-skylib that referenced this issue Jan 16, 2020
It's on by default since Bazel 2.0, and the flag is now going away.

bazelbuild/bazel#7130
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.