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 workspace.exclude key #3837

Merged
merged 1 commit into from
Mar 23, 2017
Merged

Conversation

alexcrichton
Copy link
Member

This commit adds a new key to the Cargo.toml manifest, workspace.exclude.
This new key is a list of strings which is an array of directories that are
excluded from the workspace explicitly. This is intended for use cases such as
vendoring where path dependencies into a vendored directory don't want to pull
in the workspace dependencies.

There's a number of use cases mentioned on #3192 which I believe should all be
covered with this strategy. At a bare minimum it should suffice to exclude
every directory and then just explicitly whitelist crates through members
through inclusion, and that should give precise control over the structure of a
workspace.

Closes #3192

This commit adds a new key to the `Cargo.toml` manifest, `workspace.exclude`.
This new key is a list of strings which is an array of directories that are
excluded from the workspace explicitly. This is intended for use cases such as
vendoring where path dependencies into a vendored directory don't want to pull
in the workspace dependencies.

There's a number of use cases mentioned on rust-lang#3192 which I believe should all be
covered with this strategy. At a bare minimum it should suffice to `exclude`
every directory and then just explicitly whitelist crates through `members`
through inclusion, and that should give precise control over the structure of a
workspace.

Closes rust-lang#3192
@rust-highfive
Copy link

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

Root { members: Option<Vec<String>> },
Root {
members: Option<Vec<String>>,
exclude: Vec<String>,
Copy link
Member

@matklad matklad Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should check that exclude is a relative path, to be conservative? Absolute paths probably do not make sense in this context.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could yeah, but we don't have checks like that for members and other such keys. I could imagine that in debugging situations it's useful to have absolute paths (although certainly never committed)

@matklad
Copy link
Member

matklad commented Mar 22, 2017

Implementation looks good to me, although I would probably try to use &[String] instead of &Option<Vec<String>>.

I wonder if we want to handle vendoring scenario automatically? That is, add directories of the replacement sources to exclude without any explicit user action.

@brson
Copy link
Contributor

brson commented Mar 23, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Mar 23, 2017

📌 Commit 67364ba has been approved by brson

@bors
Copy link
Contributor

bors commented Mar 23, 2017

⌛ Testing commit 67364ba with merge 4e95c6b...

bors added a commit that referenced this pull request Mar 23, 2017
Add a workspace.exclude key

This commit adds a new key to the `Cargo.toml` manifest, `workspace.exclude`.
This new key is a list of strings which is an array of directories that are
excluded from the workspace explicitly. This is intended for use cases such as
vendoring where path dependencies into a vendored directory don't want to pull
in the workspace dependencies.

There's a number of use cases mentioned on #3192 which I believe should all be
covered with this strategy. At a bare minimum it should suffice to `exclude`
every directory and then just explicitly whitelist crates through `members`
through inclusion, and that should give precise control over the structure of a
workspace.

Closes #3192
@bors
Copy link
Contributor

bors commented Mar 23, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: brson
Pushing 4e95c6b to master...

@bors bors merged commit 67364ba into rust-lang:master Mar 23, 2017
@alexcrichton alexcrichton deleted the workspace-exlucde branch March 23, 2017 23:18
@alexcrichton
Copy link
Member Author

@matklad oh I'm fine either way with &[String] vs &Option<...>, this was just easier to implement at the beginning.

I think it's ok though to avoid the vendoring scenario. It would only arise if you had a path dependency into a vendor dir which is a serious no no, you're supposed to have "crates.io deps" which get resolved to the vendor dir.

@matklad
Copy link
Member

matklad commented Mar 25, 2017

It would only arise if you had a path dependency into a vendor dir which is a serious no no, you're supposed to have "crates.io deps" which get resolved to the vendor dir.

I've hit this when I've tried opening a vendored dependencies as a separate project in IntelliJ, because executing cargo metadata inside the vendored package fails due to invalid workspace configuration.

But this is a rather obscure edge case. If you open the main project in IntelliJ, poking vendored deps works just fine.

@alexcrichton
Copy link
Member Author

Hm ok, if it becomes a problem then I think we can definitely update this to take vendoring into account, lemmek now if that becomes necessary!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Workspaces: Add a way to disable transitive inclusion of path dependencies
6 participants