workspaces: exclude not working in Rustbuild

[nrc]

See rust-lang/rust#42146 (comment)

[nrc]

ping @alexcrichton - it seems like you have a patch that fixes this, but it never landed. Did it work?

[alexcrichton]

I created a PR at #4297, got bogged down in review, and @behnam kindly offered to take over when working with #4268 as well.

[behnam]

Right. Sorry for the delay here. I'm back on cargo issues and will send out a PR for this soon.

[alexcrichton]

No worries! Just wanted to check in :)

[behnam]

@nrc, I was looking at the original thread, specially your last comment there: https://github.com/rust-lang/rust/pull/42146/files#r119265362

I'm not sure how to repro the error on my machine. Which paths need to BE members of the workspace, and which ones should NOT be? What command gives you the error you mentioned?

The reason I'm trying to repro the error is to see what's desired in this case is actually going to be addressed by the solution I have recommended here.

[alexcrichton]

@benham these lines should all be exclude = ["tools/rls/test_data"] and I think you can reproduce it via ./x.py test src/tools/rls

[behnam]

I got back to this again today and realized the implementation for dealing with workspace configs is a bit too intertwined. I need to spend more time on it, and it may need a bit of refactoring around WorkspaceConfig. Would that be okay?

I also want to confirm the solution: based on the last discussion, the plan is to have workspace.members to be a list of glob patterns, and workspace.exclude a list of gitignore patterns. But, it's not clear which one takes priority: members or exclude?

[alexcrichton]

@behnam refactoring sounds great!

I was thinking something like the "longest pattern" wins, but if it's ambiguous we can just pick one

[behnam]

I'm finally getting to a clear implementation here, that hopefully will be stable for a while.

There's only one case that I cannot make a clear cut myself---since I haven't used workspace.exclude that much in my projects and not sure which outcome is more expected in other projects---and would like your comments on.

Let's say we have these:

Virtual Root: root/Cargo.toml:

[workspace]
members = ["foo"]
exclude = ["foo/bar"]

Package: root/foo/Cargo.toml:

[package]
# ...

[dependencies]
"bar" = { version = ..., path = "bar" }

Package: root/foo/bar/Cargo.toml

[package]
# ...

What's happening here is that the workspace root only knows about one direct member, foo, and bar is resolved via being dependency of that only member.

The question is:

a) Should the exclude rule ("foo/bar") apply in this case and filter out bar from being a member of this workspace (which increases the complexities of membership resolution a bit)? or,

b) workspace.exclude is only expected to apply to the non-resolved memberships, and after the resolution of direct members, all the dependencies are applied, without looking at workspace.exclude in this case (which is the current behavior)?

AFAIU, the either of these work for the case of rust virtual repo (and the main issue brought up in rust-lang/rust#42146 (comment)).

Looking at the docs, it's not clear which one was intended in the design.

Any comments, @alexcrichton?

[behnam]

Actually, I realized that I didn't exactly state the current problem: One problem is that cargo is behaving as (b) when going top-down (when the workspace root manifest is active) and behaving as (a) when going bottom-up (when a member manifest is active and checking if it's a member of the root or not).

[alexcrichton]

@behnam hm an interesting question... I'd probably expect (a) but if that causes too many complications it seems fine to go with (b), these two strategies some mostly equivalent to me I think?

[stale]

As there hasn't been any activity here in over 6 months I've marked this as stale and if no further activity happens for 7 days I will close it.

I'm a bot so this may be in error! If this issue should remain open, could someone (the author, a team member, or any interested party) please comment to that effect?

The team would be especially grateful if such a comment included details such as:

• Is this still relevant?
• If so, what is blocking it?
• Is it known what could be done to help move this forward?

Thank you for contributing!

(The cargo team is currently evaluating the use of Stale bot, and using #6035 as the tracking issue to gather feedback.)

If you're reading this comment from the distant future, fear not if this was closed automatically. If you believe it's still an issue please leave a comment and a team member can reopen this issue. Opening a new issue is also acceptable!

[dwijnand]

Fixed by #4594.