Skip to content

Add a workspace.exclude key#3837

Merged
bors merged 1 commit into
rust-lang:masterfrom
alexcrichton:workspace-exlucde
Mar 23, 2017
Merged

Add a workspace.exclude key#3837
bors merged 1 commit into
rust-lang:masterfrom
alexcrichton:workspace-exlucde

Conversation

@alexcrichton

Copy link
Copy Markdown
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
Copy Markdown

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>,

@matklad matklad Mar 21, 2017

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

matklad commented Mar 22, 2017

Copy link
Copy Markdown
Contributor

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

brson commented Mar 23, 2017

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Mar 23, 2017

Copy link
Copy Markdown
Contributor

📌 Commit 67364ba has been approved by brson

@bors

bors commented Mar 23, 2017

Copy link
Copy Markdown
Contributor

⌛ 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

bors commented Mar 23, 2017

Copy link
Copy Markdown
Contributor

☀️ 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
Copy Markdown
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

matklad commented Mar 25, 2017

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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