-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Include unpublishable path dependencies of the root package for publishing #4735
Conversation
Before this patch, all path deps must be published before the root crate is published to the registry. This patch allow `cargo package` to include files of path dependencies in the generated package if the dependencies have the manifest key "package.publish" set to false, which means the crate cannot be registered as a sharable crate by itself at a registry, but still allowed to be published along with the root crate.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (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. |
cc @rust-lang/cargo So basically this allows one to have "private internal" crates, which can't be used outside of the current package? I personally would really love to have this feature: it's important to be able to create an appropriate physical layout in terms of compilation units, without exposing the details to the consumer of the package. But I am quite surprised by the fact that there's no huge demand for this feature, so perhaps it's not that important? This also is related to rfc1997 about private dependencies. |
Thanks for the pull-request @iology! I think this actually can work like this, without changing the fundamental property that Cargo package contains a single library, because all other "libraries" are strictly private and not visible from outside! But we probably need an RFC for this, because there may be some non-obvious ramifications. For example, I think that for such "internal" packages a lot of fields from Cargo.toml, most importantly |
@matklad You are right. There may still be some problems about rustdoc and I haven't added more tests. And I must say that I'm still a newbie of Rust, have only made some small programs. So your opinions are definitely needed! You can take this patch as just a proof of concept! |
I do agree that this needs an RFC, to specify the semantics here. That's not a hard process, and there are many people who can help you with it. It's a great way to get started with contributing to Rust. |
Is there any RFC about this topic? |
src/cargo/util/toml/mod.rs
Outdated
details.path.is_some() { | ||
match kind { | ||
// `cargo [build|test|bench]` are not affected. | ||
Some(Kind::Normal) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: currently we cannot know if the manifest is loaded for cargo install or cargo package. kind
is always None for normal dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, I don't know why I'm checking this here. The original code doesn't have this logic too. UPDATE: Oh, this does matter, the code need some refactoring.
This feature sounds very interesting to me. For example I have a crate I would also use this to fork other crates. If I need an urgent fix, but the original crate author is slow to respond, I'd publish my crate with a dependency temporarily bundled in (and keep ability to unbundle it). That would let me publish quickly without polluting crates.io with temporary full-blown forks that I don't intend to maintain. |
@pornel and everyone: I think it's better to move all the discussion to RFC at this moment :) |
☔ The latest upstream changes (presumably #4872) made this pull request unmergeable. Please resolve the merge conflicts. |
I'm going to close this for now in favor of the RFC @matklad linked to |
Rebase of: rust-lang#4735 RFC: rust-lang/rfcs#2224 Before this patch, all path deps must be published before the root crate is published to the registry. This patch allow `cargo package` to include files of path dependencies in the generated package if the dependencies have the manifest key "package.publish" set to false, which means the crate cannot be registered as a sharable crate by itself at a registry, but still allowed to be published along with the root crate. Co-authored-by: J.W <jakwings@gmail.com>
ping #4726
Before this patch, all path deps must be published before the root crate is published to the registry.
This patch allow
cargo package
to include files of path dependencies in the generated package if the dependencies have the manifest key "package.publish" set to false, which means the crate cannot be registered as a sharable crate by itself at a registry, but still allowed to be published along with the root crate.