Bzlmod-aware runfiles library by dzbarsky · Pull Request #2566 · bazelbuild/rules_rust

dzbarsky · 2024-03-20T21:01:09Z

This implements the repo_mapping capabilities and provides a new runfiles API. The new API can be accessed explicitly as runfiles::Runfiles::rlocation_from or with the runfiles::rlocation! macro, which adds compile-time support for correctly embedding the external repo. This is a purely new API, existing usage continues to work, although we mark it deprecated because it's not fully correct. We can remove it at some point in the future.

This PR also transitions in-repo examples/tests to using it, in case anyone copies them.

matts1 · 2024-03-20T22:17:27Z

 //!     ```
 //!
-//! 3.  Create a Runfiles object and use rlocation to look up runfile paths:
+//! 3.  Create a Runfiles object and use Rlocation! to look up runfile paths:


Should it be title-case? I was under the impression that naming conventions dictated that since it's a function, it should be rlocation

sure, I wasn't sure what the convention was for macros but I think you're right

matts1 · 2024-03-20T22:30:03Z

    pub fn create() -> io::Result<Self> {
-        if is_manifest_only() {
+        let mode = if is_manifest_only() {
            Self::create_manifest_based()


IIRC @fmeum said that the canonical implementation is at rules_python and that other implementations should seek to follow that. In all other implementations, we create the manifest based one if RUNFILES_MANIFEST_FILE exists, otherwise we create it based on RUNFILES_DIR.

I believe we should instead have:

let mode = if let Ok(manifest_file) = std::env::var(MANIFEST_FILE_ENV_VAR) { create_manifest_based(Path::from(manifest_file))? } else { Mode::DirectoryBased(find_runfiles_dir()?) }

I'm happy to swap to that. It's a bit riskier since it's a behavior change but it's more correct you point out

RUNFILES_MANIFEST_ONLY is a trap that breaks cross-platform builds and should not be used. Instead, I would say the following flow is the best you can do at this point in time:

If at least one of RUNFILES_MANIFEST_FILE or RUNFILES_DIR is set:

If RUNFILES_MANIFEST_FILE is set and exists, use it.

If RUNFILES_DIR is set and exists, use it.

Error.

If the runfiles manifest exists in one of the well-known locations, use it.

If the runfiles directory exists in one of the well-known locations, use it.

Error.

matts1 · 2024-03-20T22:44:59Z

 const TEST_SRCDIR_ENV_VAR: &str = "TEST_SRCDIR";

+#[macro_export]
+macro_rules! Rlocation {


I'm not going to say we should do it one way or the other, but we should definitely at least consider the folllowing API as an alternative:

let r = runfiles::create!() // Expands to runfiles::create(env!("REPOSITORY_NAME")) r.rlocation(path)

Pros:

More clean API (less macros)

More performant (only need to calculate the repo mapping once)

Simpler to migrate to (only migrate the runfiles constructor, not the rlocation calls)

Cons:

If you're passing the runfiles constructor across repo boundaries, it might do the wrong repo mapping. I can't see this ever happening though - I've never seen a public API that takes in runfiles, because they can just create the runfiles object themselves.

Yep I asked @fmeum the same and he mentioned passing across repo boundaries is a nice to have. I'm fine either way but let's figure it out in this thread before I refactor more :)

Re: more performant - I think it's the same? The mapping is only calculated at startup? Or if you're referring to the map lookups + string ops - you're about to do File IO, I don't think it matters :)

The migration path is also a good point. I think as-written the code doesn't require any client changes, but they can move to the new API to become more correct. I think with your proposal everyone using runfiles needs a minor code change?

Ah, good point about the migration path, I hadn't notice that we left the old method, so I thought this whole PR was backwards incompatible. If that's the case, I think we definitely just do it the way you've already done it.

dzbarsky · 2024-03-21T23:58:05Z


    # Ignore external targets
-    if target.label.workspace_root.startswith("external"):
+    if target.label.workspace_name:


driveby cleanup to not depend on external

dzbarsky · 2024-03-29T19:26:06Z

@UebelAndre this is passing tests now. I'm planning to add an external-repo integration test as well to double check, but assuming that is OK, can we merge this? None of us have a better idea how to make this work correctly and if I understand correctly the state of the world, enabling bzlmod in a repo with rust code will make rust-analyzer not work correctly, even if the rust rules are not using bzlmod. Would be good to get a fix in

illicitonion

LGTM, thanks! It'd be great to have that external repo test you mentioned.

illicitonion · 2024-04-02T16:29:28Z

+        .lines()
+        .map(|line| {
+            let parts: Vec<String> = line.splitn(3, ',').map(String::from).collect();
+            ((parts[0].clone(), parts[1].clone()), parts[2].clone())


It would be good to error rather than panic if this file is malformed and doesn't contain 3 elements

done, I also fixed the duplicative string copying while I was here

dzbarsky · 2024-04-02T18:38:24Z

LGTM, thanks! It'd be great to have that external repo test you mentioned.

I added it as a separate commit. It's a bit complicated setup but it's what we need to really verify we are using the correct repo_mapping of transitive repos instead of just the main one

UebelAndre · 2024-04-03T01:59:53Z

Does this break the existing API? Can you update the PR description to reflect the changes some more?

illicitonion

Works for me! Will leave for @UebelAndre to review/merge when happy

dzbarsky · 2024-04-03T15:04:02Z

Does this break the existing API? Can you update the PR description to reflect the changes some more?

No, this is a new API to make this non-breaking, and the existing API is marked deprecated. The PR summary already mentioned that, but I expanded it a bit, hopefully it's clearer now!

dzbarsky · 2024-04-09T02:15:54Z

@UebelAndre does the PR description make sense now? Anything else you'd like to see before this lands?

dzbarsky · 2024-04-21T14:24:43Z

@UebelAndre @illicitonion What can I do to get this unstuck and landed?

This PR is blocking other issues, such as #2615.

There have been other attempts to fix this for over a year now, such as: first one, second one.

I think I have spent more time trying to get this PR landed than it took to write the code, which is a little frustrating as you can imagine.

scentini · 2024-04-22T08:19:23Z

This PR LGTM, and as it was already approved by @illicitonion a while back, I'm going to to ahead and merge it. @UebelAndre please feel free to do post-merge review, and we'll follow up.

Dismissing review to enable merge when ready. The PR author has addressed the reviewer's comments.

UebelAndre · 2024-04-29T03:40:36Z

Sorry, this one fell through the cracks. I didn't mean to delay the change!

dzbarsky mentioned this pull request Mar 20, 2024

Make runfiles library bzlmod compatible #2440

Closed

dzbarsky force-pushed the main branch 3 times, most recently from 82eab55 to 6236ef1 Compare March 20, 2024 21:24

matts1 reviewed Mar 20, 2024

View reviewed changes

dzbarsky force-pushed the main branch 3 times, most recently from cb7a2bf to c719e80 Compare March 21, 2024 15:27

UebelAndre previously requested changes Mar 21, 2024

View reviewed changes

Comment thread rust/private/rustc.bzl

dzbarsky force-pushed the main branch from c719e80 to 30159c1 Compare March 21, 2024 17:19

dzbarsky commented Mar 21, 2024

View reviewed changes

dzbarsky force-pushed the main branch from 4ccfbde to ebe4f80 Compare March 22, 2024 00:24

dzbarsky mentioned this pull request Mar 22, 2024

Support repo mapping for bazel runfiles. #1847

Closed

fmeum approved these changes Mar 29, 2024

View reviewed changes

illicitonion reviewed Apr 2, 2024

View reviewed changes

dzbarsky added 4 commits April 2, 2024 12:44

Bzlmod-aware runfiles library

4464f20

Remove runfiles.current_repository()

1d7ede7

Cleanup more runfiles references

a275ba6

Review feedback

c437c0f

dzbarsky force-pushed the main branch from b9f617d to 486ad4e Compare April 2, 2024 18:37

dzbarsky force-pushed the main branch 2 times, most recently from f962dcb to 5ef713a Compare April 2, 2024 18:41

Add test

e5a57b7

dzbarsky force-pushed the main branch from 5ef713a to e5a57b7 Compare April 2, 2024 18:48

Fix usage in examples and tests

a6fb2ac

dzbarsky force-pushed the main branch from 79ebf3d to a6fb2ac Compare April 2, 2024 19:31

buildifier

5e0a723

dzbarsky requested a review from UebelAndre April 2, 2024 19:59

matts1 approved these changes Apr 3, 2024

View reviewed changes

illicitonion approved these changes Apr 3, 2024

View reviewed changes

dzbarsky mentioned this pull request Apr 13, 2024

gen_rust_project doesn't work with bzlmod #2615

Closed

Merge branch 'main' into main

fb25a90

scentini enabled auto-merge April 22, 2024 08:19

scentini disabled auto-merge April 22, 2024 08:21

scentini enabled auto-merge April 22, 2024 08:21

scentini added this pull request to the merge queue Apr 22, 2024

Merged via the queue into bazelbuild:main with commit 8d074a0 Apr 22, 2024

Conversation

dzbarsky commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matts1 Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzbarsky Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzbarsky commented Mar 29, 2024

Uh oh!

illicitonion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzbarsky commented Apr 2, 2024

Uh oh!

UebelAndre commented Apr 3, 2024

Uh oh!

illicitonion left a comment

Choose a reason for hiding this comment

Uh oh!

dzbarsky commented Apr 3, 2024

Uh oh!

dzbarsky commented Apr 9, 2024

Uh oh!

dzbarsky commented Apr 21, 2024

Uh oh!

scentini commented Apr 22, 2024

Uh oh!

UebelAndre commented Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dzbarsky commented Mar 20, 2024 •

edited

Loading

matts1 Mar 20, 2024 •

edited

Loading

dzbarsky Mar 21, 2024 •

edited

Loading

UebelAndre commented Apr 29, 2024 •

edited

Loading