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

Include unpublishable path dependencies of the root package for publishing #4735

Closed
wants to merge 8 commits into from
Closed

Conversation

jakwings
Copy link

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.

J.W added 2 commits November 21, 2017 16:10
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.
@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 @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.

@matklad
Copy link
Member

matklad commented Nov 21, 2017

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.

@matklad
Copy link
Member

matklad commented Nov 21, 2017

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 version, are irrelevant. So perhaps such "private packages" should have more first-class support in Cargo?

@jakwings
Copy link
Author

@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!

@joshtriplett
Copy link
Member

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.

@jakwings
Copy link
Author

Is there any RFC about this topic? package.publish=false may carry too much semantics, so another attribute may be needed to override package.publish.

details.path.is_some() {
match kind {
// `cargo [build|test|bench]` are not affected.
Some(Kind::Normal) => {
Copy link
Author

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.

Copy link
Author

@jakwings jakwings Nov 22, 2017

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.

@kornelski
Copy link
Contributor

kornelski commented Nov 26, 2017

This feature sounds very interesting to me. For example I have a crate c3 that uses a sub crate to contain pains of building and patching clang+llvm (https://crates.io/crates/c3_clang_extensions), so I had to publish it, but it's not useful outside of its parent.

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.

@matklad
Copy link
Member

matklad commented Nov 26, 2017

@pornel and everyone: I think it's better to move all the discussion to RFC at this moment :)

rust-lang/rfcs#2224

@bors
Copy link
Contributor

bors commented Dec 29, 2017

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

@alexcrichton
Copy link
Member

I'm going to close this for now in favor of the RFC @matklad linked to

haraldh added a commit to haraldh/cargo that referenced this pull request Aug 21, 2020
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>
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.

7 participants