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

Add a 'worskpace.default-packages' config to override implied --all? #4507

Closed
SimonSapin opened this issue Sep 18, 2017 · 8 comments · Fixed by #4743
Closed

Add a 'worskpace.default-packages' config to override implied --all? #4507

SimonSapin opened this issue Sep 18, 2017 · 8 comments · Fixed by #4743
Labels
A-workspaces Area: workspaces

Comments

@SimonSapin
Copy link
Contributor

Since #4335, --all is implied for virtual workspaces. This is not appropriate in some cases (like Servo) for two reasons:

  • Two of the "top-level" crates (servo and geckolib) both depend on the same library (style) and each enable a corresponding Cargo feature. These features are mutually exclusive. (Compiling style only works when exactly one of these two "modes" is enabled.)
  • --all includes all transitive path dependencies, even though one of those (task_info) is normally only included through a [target.'cfg(target_os = "macos")'.dependencies] section (and doesn’t compile on other platforms).

Both of these are probably unusual, so #4335 is still a good default. But I’d like there to be a way to opt into a different behavior. It could be something like this:

[workspace]
default-packages = ["servo"]

Running e.g. cargo build at the top level without more arguments would behave like cargo build -p servo instead of cargo build --all.

@matklad
Copy link
Member

matklad commented Sep 18, 2017

Previously cargo build would just fail in your use case with "this command was invoked with virtual manifest and requires a real one", is that right? So nothing has actually been broken, and this is just a feature request?

Or has #4335 broke some workflow which used to work?

@matklad
Copy link
Member

matklad commented Sep 18, 2017

These features are mutually exclusive. (Compiling style only works when exactly one of these two "modes" is enabled.)

This is #4463

@alexcrichton
Copy link
Member

Sounds reasonable to me!

@alexcrichton alexcrichton added the A-workspaces Area: workspaces label Sep 18, 2017
@SimonSapin
Copy link
Contributor Author

@matklad Correct, previously this would fail with that error message. Now it fails later in a build script or in rustc. This is a feature request to enable new workflows.

@alexcrichton Cool, I’ll work on a PR later.

@SimonSapin
Copy link
Contributor Author

Should the key name be default-members instead?

@matklad
Copy link
Member

matklad commented Nov 23, 2017

Should the key name be default-members instead?

FWIW, I don't especially like neither default-members nor default-packages, because the crucial information (default for what?) is missing. However, I don't know a better name, but members are better than packages :(

How should default-members interact with a workspace whose root package is not virtual? Currently, we imply --all for a virtual manifest, but for non-virtual manifest we build only the root package itself. I think default-members should be able to override this behavior.

Let's cc @rust-lang/cargo to brainstorm the better name, and to look at this feature in general :)

@SimonSapin
Copy link
Contributor Author

My work-in-progress patch does not change the behavior with non-virtual workspaces, but you’re right that using default-members there makes sense too.

@SimonSapin
Copy link
Contributor Author

Let’s move discussion to #4743.

bors added a commit that referenced this issue Nov 29, 2017
Add a workspace.default-members config that overrides implied --all

Fixes #4507.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-workspaces Area: workspaces
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants