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.

Sign up for GitHub

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

Jump to bottom

Add field private to Cargo.toml #2218

Closed
wants to merge 0 commits into from
Closed

Add field private to Cargo.toml #2218

wants to merge 0 commits into from

Conversation

JanLikar
Copy link
Contributor

private can be used to prevent a package from being accidentally published to crates.io.

[Fix #2202]

@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 @huonw (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.

alexcrichton
alexcrichton reviewed Dec 16, 2015
View reviewed changes
src/cargo/core/manifest.rs
@@ -204,6 +207,7 @@ impl Manifest {
pub fn version(&self) -> &Version { self.package_id().version() }
pub fn warnings(&self) -> &[String] { &self.warnings }
pub fn profiles(&self) -> &Profiles { &self.profiles }
pub fn private(&self) -> &bool { &self.private }
Copy link
Member

Choose a reason for hiding this comment

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

It's ok for this to just return a bool instead of &bool because it's a Copy type

@alexcrichton
Copy link
Member

cc @erickt

@alexcrichton alexcrichton assigned alexcrichton and unassigned huonw Dec 16, 2015
@erickt
Copy link
Contributor

erickt commented Dec 16, 2015

Wonderful! Thanks for implementing this!

@alexcrichton
Copy link
Member

I chatted with @wycats about this on IRC for a moment, and some interesting ideas came up:

  • The name "private" may not always be right here, sometimes it's just an internal crate that's not necessarily private.
  • The name "internal" may also not be quite right as there's no clear connection between "internal" can "can't publish".
  • The name key "allow-publish" can be super clear, but may not be extensible to other interpretations.

@JanLikar
Copy link
Contributor Author

What do you think about "publishable"? I think "private", "allow-publish" or "publishable" should be ok...

@pwoolcoc
Copy link
Contributor

what about a publish-to = [<url>, <url>, ...] key? When not specified, publish-to = [ 'https://crates.io' ] would be assumed, but you could also specify a whitelist of other destinations, or publish-to = [] to prevent publication?

@JanLikar
Copy link
Contributor Author

That sounds ok, but what happens when publish-to does not include 'crates.io' and cargo publish is run without the --host argument?

@tomaka
Copy link
Contributor

tomaka commented Dec 22, 2015

I have a suggestion: what about making cargo new set private (or whatever it is called) to true by default?

@JanLikar
Copy link
Contributor Author

@tomaka I'm not sure about this, I think most of the people would prefer to have it the other way.

@alexcrichton
Copy link
Member

Thanks for being patient @JanLikar! I had a chance to chat with @wycats today about this and we concluded that publish is likely the best option for naming here. Could you update to that? Other than that this looks good to me, thanks!

@alexcrichton alexcrichton added the relnotes Release-note worthy label Jan 23, 2016
@alexcrichton
Copy link
Member

Thanks @JanLikar! Could you also squash into one commit? Also feel free to ping a PR whenever it's updated as unfortunately github doesn't send notifications out for that :(

@JanLikar JanLikar force-pushed the private-crate branch 3 times, most recently from 496a7c8 to 84992f1 Compare January 26, 2016 14:06
@JanLikar
Copy link
Contributor Author

@alexcrichton I did it thanks to @steveklabnik.

@alexcrichton
Copy link
Member

Oh dear, was a commit lost? Right now this only contains the doc changes?

@JanLikar
Copy link
Contributor Author

I don't know how this happened...

@alexcrichton
Copy link
Member

You should be able to use git reflog to perhaps recover the original commits (so long as you're in the same repo)

@steveklabnik
Copy link
Member

It looked to me like the original branch had just one commit; that's my fault.

I have them locally, let me push up a branch.

@steveklabnik
Copy link
Member

Here's the original PR: #2320

It still looks like there's only one commit by @JanLikar in there. Maybe it was messed up before I got involved.

That said, yes, reflog should be able to find the old history...

@alexcrichton
Copy link
Member

Could you do a fetch plus a git rebase? It looks like a stray commit made its way in here as well

@JanLikar JanLikar closed this Jan 26, 2016
@JanLikar JanLikar mentioned this pull request Jan 26, 2016
Merged
@JanLikar
Copy link
Contributor Author

@alexcrichton I'm sorry, I messed something up, so the PR was closed automatically. Here's the new PR #2321.

bors added a commit that referenced this pull request Jan 26, 2016
@bors
Auto merge of #2321 - JanLikar:private-crate, r=alexcrichton
6ffd134 
I Accidentally closed #2218, this is the new PR.

[Fix #2202]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
relnotes Release-note worthy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@JanLikar @rust-highfive @alexcrichton @erickt @pwoolcoc @tomaka @steveklabnik @huonw