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

Indicate how Cargo locates the manifest #10770

Merged
merged 1 commit into from
Sep 27, 2022
Merged

Indicate how Cargo locates the manifest #10770

merged 1 commit into from
Sep 27, 2022

Conversation

wmstack
Copy link
Contributor

@wmstack wmstack commented Jun 18, 2022

Currently the only place where that is documented is in an obscure flag called --manifest-path. The behaviour of Cargo in regards to locating the manifest should be in a more obvious place, and this seems to be right about where I added it.

@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 @ehuss (or someone else) soon.

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 18, 2022
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

First, thanks for your contribution. It's glad to see hard works on doc improvement!

Your observation is correct. Cargo does search Cargo.toml in current working dir and upward if not exist. But speaking to workspaces it becomes a bit complicated. Cargo searches for root workspace manifest by either traversing parent dirs or look up the field package.workspace.

I might also be confused seeing a sentence about how Cargo locates Cargo.toml there, as the paragraph is mainly talking about the format of Cargo.toml but not any individual command. I would try to expand the description of cargo locate-project with the search logic, and refer to it in anywhere needed.

@wmstack
Copy link
Contributor Author

wmstack commented Jul 2, 2022

Would it be alright to reference cargo locate-project in the manifest? I'd imagine that it wouldn't occur to a normal user to look there, so there needs to be a reference, even a short one, that indicates how the manifest is located somewhere obvious. Or, which directory would you prefer it was referenced in?

It is just that I fear that such knowledge should become tribal knowledge, and just having the information in locate-project but not referenced anywhere where the user would most likely look for, in my opinion, is not ideal. I suggest that the manifest be updated with a heading on such auxiliary information, before getting into the actual manifest format. For example, the manifest mentions that the Cargo.toml file is written in the TOML format, which is a no-brainer, yet it is there for completeness, but fails to mention how Cargo actually finds it, which is more important to the subject at hand.

@wmstack
Copy link
Contributor Author

wmstack commented Jul 2, 2022

Just a general question. Whose job is it to resolve merge conflicts when pull requests diverge from the main branch? Is it the contributor or the maintainer? Does that generally depend on the particular open-source project?

@weihanglo
Copy link
Member

I would expect something like this, and then we can expand the detail on cargo-locate-project. My writing is rusty. Feel free to polish it :)

The `Cargo.toml` file for each package is called its *manifest*. It contains
the metadata that Cargo needs to compile a package. Most Cargo subcommands
start from  locating the manifest file first, and then perform subsequent
operations. See `cargo locate-project` chapter for more detail on how cargo
finds the manifest file.

The file is written in the [TOML] format. Every manifest file consists of the following sections:

Just a general question. Whose job is it to resolve merge conflicts when pull requests diverge from the main branch? Is it the contributor or the maintainer? Does that generally depend on the particular open-source project?

I would say it's PR authors' responsibilities unless the conflict is too complicated. If in that case, maintainers and contributors collaborate to resolve it. And I feel like only when it is really a conflict should you update your branch to master.

@weihanglo weihanglo added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2022
@weihanglo
Copy link
Member

Ping @wmstack. Just checking in to see if you are still interested in working on this, or if you had any questions.

@weihanglo
Copy link
Member

Hi. Looks like you're still working on this PR. The PR looks good, but it is not much helpful inkt linking to cargo locate-project without updating doc there. Are you willing to add docs for cargo locate-project? Or do you have any other thought on this?

r? @weihanglo

@rust-highfive rust-highfive assigned weihanglo and unassigned ehuss Sep 26, 2022
@wmstack
Copy link
Contributor Author

wmstack commented Sep 27, 2022

Hmm, I was under the impression that I had to run a shell script to fix failing unit tests. However, when I did that, it seems that I lost the documentation that I put there at 755bdc1

@weihanglo
Copy link
Member

weihanglo commented Sep 27, 2022

Sorry for not saying in clear. To generate docs for commands, you need to follow this guide. That is, edit src/doc/man/cargo-locate-project.md and run src/doc/build-man.sh.

We're almost there! Only a few things to do before merging:

  • Could you try adding some words in cargo-locate-project doc to tell the difference that other commands usually locate for workspace first by default, whereas cargo locate-project need the --workspace flag?
  • Could you do a clean-up for commits, such like rebasing this PR onto master and keeping only meaningful commits?

Thank you!

@wmstack
Copy link
Contributor Author

wmstack commented Sep 27, 2022

Every time I run the shell script, the content that I have written in cargo-locate-project dissappears...

Edit: I see the man pages form the commands section of the cargo book, which are auto-generated. That means I needed to edit the man pages. The manifest, not being part of the man pages, was not deleted since it was not auto-generated. I was wondering why the markdown looked a bit generated...

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Every time I run the shell script, the content that I have written in cargo-locate-project dissappears...

You need to edit src/doc/man/cargo-locate-project.md instead and generate others.

src/doc/src/commands/cargo-locate-project.md Outdated Show resolved Hide resolved
@wmstack
Copy link
Contributor Author

wmstack commented Sep 27, 2022

Okay I fixed the typos and squashed all the commits into 1.

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thank you @wmstack. It's my fault but can you do me one more favor for this?
#10770 (comment)

@wmstack
Copy link
Contributor Author

wmstack commented Sep 27, 2022

Okay, I removed the last comment.

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thank you so much!!

@weihanglo
Copy link
Member

Oh no. We need to run src/doc/build-man.sh again.

@wmstack
Copy link
Contributor Author

wmstack commented Sep 27, 2022

Man, I had to touch up on skills I hadn't used in ages! Like reflog and rebase and such. Thanks as well!

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 27, 2022

📌 Commit 1a88627 has been approved by weihanglo

It is now in the queue for this repository.

@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-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Sep 27, 2022
@bors
Copy link
Contributor

bors commented Sep 27, 2022

⌛ Testing commit 1a88627 with merge 76f080b...

@bors
Copy link
Contributor

bors commented Sep 27, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 76f080b to master...

@bors bors merged commit 76f080b into rust-lang:master Sep 27, 2022
bors pushed a commit to rust-lang-ci/rust that referenced this pull request Sep 27, 2022
22 commits in 73ba3f35e0205844418260722c11602113179c4a..f5fed93ba24607980647962c59863bbabb03ce14
2022-09-18 06:38:16 +0000 to 2022-09-27 12:03:57 +0000

- build-scripts.md: Use em dash consistently. (rust-lang/cargo#11150)
- Indicate how Cargo locates the manifest (rust-lang/cargo#10770)
- Reduce references to `[project]` within cargo (rust-lang/cargo#11135)
- Iteratively construct target cfg (rust-lang/cargo#11114)
- update comment about `CARGO_BIN_EXE_` (rust-lang/cargo#11146)
- Call out that not all config values can be set via env vars (rust-lang/cargo#11139)
- Bump to 0.67.0, update changelog (rust-lang/cargo#11137)
- ci: update toolchain for building api doc (rust-lang/cargo#11134)
- Http publish not noop (rust-lang/cargo#11111)
- Improve errors for TOML fields that support workspace inheritance (rust-lang/cargo#11113)
- switch to `std::task::ready!()` where possible (rust-lang/cargo#11130)
- Report cmd aliasing failure with more contexts (rust-lang/cargo#11087)
- minor: remove unused mut (rust-lang/cargo#11127)
- fix(cli): Forward non-UTF8 arguments to external subcommands (rust-lang/cargo#11118)
- This change adds an example to the authors attribute in the manifest. (rust-lang/cargo#10938)
- Add support for relative git submodule paths (rust-lang/cargo#11106)
- make unknown features on `cargo add` more discoverable (rust-lang/cargo#11098)
- Unlink old final artifacts before compilation (rust-lang/cargo#11122)
- refactor(cli): Prepare for clap v4 (rust-lang/cargo#11116)
- fix(cli): Error trailing args rather than ignore (rust-lang/cargo#11119)
- Add a minor clarification (rust-lang/cargo#11093)
- doc(changelog): mention CVE fixes (rust-lang/cargo#11104)
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 27, 2022
Update cargo

22 commits in 73ba3f35e0205844418260722c11602113179c4a..f5fed93ba24607980647962c59863bbabb03ce14 2022-09-18 06:38:16 +0000 to 2022-09-27 12:03:57 +0000

- build-scripts.md: Use em dash consistently. (rust-lang/cargo#11150)
- Indicate how Cargo locates the manifest (rust-lang/cargo#10770)
- Reduce references to `[project]` within cargo (rust-lang/cargo#11135)
- Iteratively construct target cfg (rust-lang/cargo#11114)
- update comment about `CARGO_BIN_EXE_` (rust-lang/cargo#11146)
- Call out that not all config values can be set via env vars (rust-lang/cargo#11139)
- Bump to 0.67.0, update changelog (rust-lang/cargo#11137)
- ci: update toolchain for building api doc (rust-lang/cargo#11134)
- Http publish not noop (rust-lang/cargo#11111)
- Improve errors for TOML fields that support workspace inheritance (rust-lang/cargo#11113)
- switch to `std::task::ready!()` where possible (rust-lang/cargo#11130)
- Report cmd aliasing failure with more contexts (rust-lang/cargo#11087)
- minor: remove unused mut (rust-lang/cargo#11127)
- fix(cli): Forward non-UTF8 arguments to external subcommands (rust-lang/cargo#11118)
- This change adds an example to the authors attribute in the manifest. (rust-lang/cargo#10938)
- Add support for relative git submodule paths (rust-lang/cargo#11106)
- make unknown features on `cargo add` more discoverable (rust-lang/cargo#11098)
- Unlink old final artifacts before compilation (rust-lang/cargo#11122)
- refactor(cli): Prepare for clap v4 (rust-lang/cargo#11116)
- fix(cli): Error trailing args rather than ignore (rust-lang/cargo#11119)
- Add a minor clarification (rust-lang/cargo#11093)
- doc(changelog): mention CVE fixes (rust-lang/cargo#11104)
@ehuss ehuss added this to the 1.66.0 milestone Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants