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

Flip --legacy_external_runfiles to false #23574

Closed
lberki opened this issue Sep 10, 2024 · 9 comments
Closed

Flip --legacy_external_runfiles to false #23574

lberki opened this issue Sep 10, 2024 · 9 comments
Assignees
Labels
breaking-change-8.0 incompatible-change Incompatible/breaking change P1 I'll work on this now. (Assignee required) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.

Comments

@lberki
Copy link
Contributor

lberki commented Sep 10, 2024

Motivation

This flag has been there since 2016 so we should make up our minds and I think it's better for everyone to do so.

Technically, it makes flipping --experimental_sibling_repository_layout somewhat more complicated since it makes the layout of the runfiles tree inconsistent with that of the execroot, but in practice, the plan is to flip them very close to each other temporally, so it doesn't matter much.

Description

This incompatible change changes runfiles trees: files in external repositories are currently available at both $RUNFILES/<main repo>/external/<external repo>/<path> and $RUNFILES/<external repo>/<path>. Flipping this flag removes the former.

The reason why the flag is not prefixed with --incompatible_* is that it predates that nomenclature.

Incompatible Flag

--legacy_external_runfiles

Migration Guide

If you are using the runfiles library, there isn't anything to do. If not, you'll have to update code to look for the files in their non-legacy location in the runfiles tree.

In particular, if --experimental_sibling_repository_layout is unset (which is the default), the path of source files in external repositories will be different between runfiles trees ($RUNFILES</repo name>/<repo-relative-path>) and the execroot (external/<repo name>/<repo-relative-path>). This means that some Make variables (e.g. $(JAVA)) cannot be used to reference the runfiles tree.

However, pseudo-Make variables ($(rlocationpath) and $(rootpath)) do work.

In which Bazel LTS version will this incompatible change be enabled?

Bazel 8

Additional Context

No response

TODO List

No response

@lberki lberki added untriaged incompatible-change Incompatible/breaking change labels Sep 10, 2024
@meteorcloudy meteorcloudy added P1 I'll work on this now. (Assignee required) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. breaking-change-8.0 and removed untriaged labels Sep 10, 2024
@peakschris
Copy link

How should we set up our .bazelrc files in the 7.3 series in order to prepare for this flip?

Is this correct?

common --nolegacy_external_runfiles
common --experimental_sibling_repository_layout

@meteorcloudy
Copy link
Member

That is correct!

@meteorcloudy meteorcloudy changed the title Flip --legacy_external_runfiles Flip --legacy_external_runfiles to false Sep 10, 2024
@fmeum
Copy link
Collaborator

fmeum commented Sep 11, 2024

Just a note: Flipping this separately from --experimental_sibling_repository_layout would make it more breaking as Make variables with exec paths (e.g. JAVA) would no longer work for runfiles discovery. Maybe Make variables should be called out in the migration notes?

@fmeum
Copy link
Collaborator

fmeum commented Sep 12, 2024

Referencing my reply to the other thread here: #23576 (comment)

@meteorcloudy
Copy link
Member

meteorcloudy commented Sep 12, 2024

@fmeum @lberki Continue the discussion about whether we should flip this flag or revert back to "legacy runfiles only" mode.

A quick test with Bazel's own test suite:

I think flipping this flag could still cause some downstream projects to break, but it's probably manageable, however, not the other way around.

@lberki
Copy link
Contributor Author

lberki commented Sep 13, 2024

@meteorcloudy that's a strong argument for flipping this flag instead of keeping the legacy paths. Should I send out a change?

@meteorcloudy
Copy link
Member

@lberki I already have one, sending it to you

copybara-service bot pushed a commit that referenced this issue Sep 13, 2024
Working towards: #23574

PiperOrigin-RevId: 674247156
Change-Id: I082ca436732cbd10cde613344566570f3cf1d424
@meteorcloudy
Copy link
Member

Closing this one since the flag is now flipped and there is no risk of rolling back.

@lberki
Copy link
Contributor Author

lberki commented Sep 19, 2024

🎉 Thanks for following through with this!

copybara-service bot pushed a commit that referenced this issue Dec 10, 2024
Migrate Bazel for #23574

PiperOrigin-RevId: 704687334
Change-Id: I47fe34775959d1b4ab4e6d931ddc6df256f6e59a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change-8.0 incompatible-change Incompatible/breaking change P1 I'll work on this now. (Assignee required) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

No branches or pull requests

7 participants