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 support for workspace.metadata table #8323

Merged
merged 7 commits into from
Jun 23, 2020

Conversation

naerbnic
Copy link
Contributor

@naerbnic naerbnic commented Jun 3, 2020

Implements feature request #8309

Additionally includes the information in the output of "cargo metadata" through a new top-level field metadata, similar to the per-package metadata field

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 3, 2020
@alexcrichton
Copy link
Member

Thanks for this! I think it's fine to not parse the values here, though, and probably treat this the same way as [package.metadata] (I think we ignore that, right?). I believe the main use case for this is to avoid Cargo warning about unused manifest keys, and instead having a blessed location to find this information.

@ehuss
Copy link
Contributor

ehuss commented Jun 4, 2020

@alexcrichton I think it is necessary to load it for the cargo metadata command which exposes the data.

src/cargo/core/workspace.rs Outdated Show resolved Hide resolved
@bors
Copy link
Collaborator

bors commented Jun 5, 2020

☔ The latest upstream changes (presumably #8321) made this pull request unmergeable. Please resolve the merge conflicts.

Reword the documentation for the two `metadata` tables, and
suggest consistent usage.
@naerbnic naerbnic force-pushed the add_workspace_metadata_table branch from 2d095a5 to 341d416 Compare June 5, 2020 16:35
@naerbnic
Copy link
Contributor Author

naerbnic commented Jun 5, 2020

Resolved and pushed

@mcgoo
Copy link
Contributor

mcgoo commented Jun 5, 2020

Will there be a mechanism to get access to workspace.metadata from build.rs? Currently I see that (for example) metadeps finds package.metadata via CARGO_MANIFEST_DIR which (as I understand it) won't de doable for the workspace metadata?

@naerbnic
Copy link
Contributor Author

naerbnic commented Jun 6, 2020

Will there be a mechanism to get access to workspace.metadata from build.rs? Currently I see that (for example) metadeps finds package.metadata via CARGO_MANIFEST_DIR which (as I understand it) won't de doable for the workspace metadata?

To my understanding, this should still be available through the cargo metadata command. The build script is provided with the CARGO environment variable, which contains the cargo binary being run. This PR adds code to include the workspace-level metadata in the output of that command.

@mcgoo
Copy link
Contributor

mcgoo commented Jun 6, 2020

Oh, I see, thanks.

One observation - assuming I understand this, a crate downloaded by cargo as part of the build will not have access to the workspace metadata that it had in it's original source tree since it is no longer in a workspace.

The [workspace|package].metadata is primarily aimed at tooling rather than build.rs I guess, so what I'm talking about here seems out of scope. Thanks again for your help.

src/cargo/core/workspace.rs Outdated Show resolved Hide resolved
@alexcrichton alexcrichton added the T-cargo Team: Cargo label Jun 9, 2020
@alexcrichton
Copy link
Member

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jun 9, 2020

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Jun 9, 2020
src/doc/src/reference/manifest.md Outdated Show resolved Hide resolved
src/doc/src/reference/workspaces.md Outdated Show resolved Hide resolved
@rfcbot rfcbot added the final-comment-period FCP — a period for last comments before action is taken label Jun 11, 2020
@rfcbot
Copy link
Collaborator

rfcbot commented Jun 11, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period An FCP proposal has started, but not yet signed off. label Jun 11, 2020
naerbnic and others added 2 commits June 11, 2020 17:01
Remove anchor to use the mdbook generated link instead.

Co-authored-by: Eric Huss <eric@huss.org>
Add a link to the mdbook generated link

Co-authored-by: Eric Huss <eric@huss.org>
@rfcbot
Copy link
Collaborator

rfcbot commented Jun 21, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@rfcbot rfcbot removed the final-comment-period FCP — a period for last comments before action is taken label Jun 21, 2020
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Jun 23, 2020

📌 Commit 16f3b8d has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 23, 2020
@bors
Copy link
Collaborator

bors commented Jun 23, 2020

⌛ Testing commit 16f3b8d with merge 78c1671...

@bors
Copy link
Collaborator

bors commented Jun 23, 2020

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 78c1671 to master...

@bors bors merged commit 78c1671 into rust-lang:master Jun 23, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 24, 2020
Update cargo

9 commits in 089cbb80b73ba242efdcf5430e89f63fa3b5328d..c26576f9adddd254b3dd63aecba176434290a9f6
2020-06-15 14:38:34 +0000 to 2020-06-23 16:21:21 +0000
- Adding environment variable CARGO_PKG_LICENSE_FILE (rust-lang/cargo#8387)
- Enable "--target-dir" in "cargo install" (rust-lang/cargo#8391)
- Add support for `workspace.metadata` table (rust-lang/cargo#8323)
- Fix overzealous `clean -p` for reserved names. (rust-lang/cargo#8398)
- Fix order-dependent feature resolution. (rust-lang/cargo#8395)
- Correct mispelling of `cargo`. (rust-lang/cargo#8389)
- Add missing license field. (rust-lang/cargo#8386)
- Adding environment variable CARGO_PKG_LICENSE (rust-lang/cargo#8325)
- Cut down on data fetch from git dependencies (rust-lang/cargo#8363)
@ehuss ehuss mentioned this pull request Jul 4, 2020
bors added a commit that referenced this pull request Jul 5, 2020
Update metadata man page.

The man pages needed to be rebuilt after #8323.
@ehuss ehuss added this to the 1.46.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants