-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add $(rlocationpath(s) ...)
expansion
#16653
Conversation
The new location expansion pattern `rlocationpath` and its plural version `rlocationpaths` resolve to the path(s) suitable for the Rlocation function offered by runfiles libraries. Compared to the `rootpath` pattern, which can returns `../repo_name/pkg/file` for a file in an external repository and `pkg/file` for a file in the main repository, the path returned by `rlocationpath` is always of the form `repo_name/pkg/file`. RELNOTES: The new path variable `$(rlocationpath ...)` and its plural form `$(rlocationpaths ...)` can be used to expand labels to the paths accepted by the `Rlocation` function of runfiles libraries. This is the preferred way to access data dependencies at runtime and works on all platforms, even when runfiles are not enabled (e.g., on Windows by default). Work towards bazelbuild#16124 Fixes bazelbuild#10923 Closes bazelbuild#16428. PiperOrigin-RevId: 485930083 Change-Id: I85c0ef29c8332eeabd8473d2611ede546eb24016
fdcfc51
to
92724d3
Compare
Hey @fmeum, I'm working to import rules_kotlin to central registry. What I noticed yesterday is that it's better that I use What would happen if we modify My guess is that this would break some users who are not using it only for runfiles and are glueing the results. I don't feel sorry for that. It would allow us to remove I'm sure there are some problems with such a plan that you already know about. I'm just thinking it would be nice to have one function for execpath and one function for runfiles path. (location is already deprecated afaik). |
@comius The main reason I dislike this is that One could argue that the Since this is so subtle, I imagine quite a number of people to actually rely on glueing stuff together that happens to work. If we break this expectation in the name of simplifying a migration, we might actually make it more difficult. I would rather deprecate Now that I think about it more, if Regarding what to do about rules_kotlin in the meantime: Could you link to an example of runfiles usage in rules_kotlin that you would like to migrate? Your options that work with both Bazel <6 and Bazel 6 include:
|
I pushed bazelbuild/rules_kotlin#860, I'm quite happy with current state. I think it's an improvement over previous state of rules_kotlin. The tests pass with old bazel, new bazel with and without bzlmod. The runfiles library in Kotlin isn't correct. I'm re-using the I also needed FilesProvider's filesToBuild in data runfiles - but found a neat workaround with Reading your response, if that's the case, I think we should mark |
Sounds good, although you should be able to use the Java runfiles library just fine.
Yeah, that's the best workaround I know of. Happy to see this bug go away though :-)
The docs I merged as part of this PR already suggest using |
I think re-use becomes less important when you cross language barrier :) In case you didn't notice, there's a kt version: https://github.com/bazelbuild/rules_kotlin/blob/master/src/main/kotlin/io/bazel/kotlin/builder/utils/BazelRunFiles.kt I wouldn't dare to try and reimplement annotation trick in kotlin.
cc @meteorcloudy or @Wyverald, who can tell if there's another 5.x.x release planned. |
@comius I wasn't aware of it. It does have some issues, I may look into fixing it after the Bazel 6 release. |
Probably no more minor releases on the 5.x track, though patch releases could be done if the need arises. We could do one for API deprecation, I guess -- the decision is a bit above my pay grade |
@comius Do we have to have this feature even for non-Bzlmod build? If not, I think it's OK to break Bazel 5 in the Bzlmod build, since it's experimental in 5. |
Yes, runfiles are there also without bzlmod and rlocationpath works without it. The problem I see is when There are repositories out there that want to have support for 2 LTS releases. |
I see, then I'm fine with having a 5.4.0 minor release to cherry-pick this feature to make the migration easier for users. @kshyanashree Do you think we have capacity to do a minor release at the same time, or should we wait after 6.0 settles down? |
Sure @meteorcloudy! I believe we can go ahead for 5.4.0 minor release in parallel. Thanks! |
The new location expansion pattern
rlocationpath
and its plural versionrlocationpaths
resolve to the path(s) suitable for the Rlocation function offered by runfiles libraries.Compared to the
rootpath
pattern, which can returns../repo_name/pkg/file
for a file in an external repository andpkg/file
for a file in the main repository, the path returned byrlocationpath
is always of the formrepo_name/pkg/file
.RELNOTES: The new path variable
$(rlocationpath ...)
and its plural form$(rlocationpaths ...)
can be used to expand labels to the paths accepted by theRlocation
function of runfiles libraries. This is the preferred way to access data dependencies at runtime and works on all platforms, even when runfiles are not enabled (e.g., on Windows by default).Work towards #16124
Fixes #10923
Closes #16629.
PiperOrigin-RevId: 485930083
Change-Id: I85c0ef29c8332eeabd8473d2611ede546eb24016