Skip to content

Block publishing on dirty directories #2714

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

Closed
wants to merge 1 commit into from
Closed

Block publishing on dirty directories #2714

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/bin/publish.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ pub struct Options {
flag_token: Option<String>,
flag_manifest_path: Option<String>,
flag_verbose: Option<bool>,
flag_allow_untracked: bool,
flag_quiet: Option<bool>,
flag_color: Option<String>,
flag_no_verify: bool,
Expand All @@ -26,6 +27,7 @@ Options:
--no-verify Don't verify package tarball before publish
--manifest-path PATH Path to the manifest of the package to publish
-v, --verbose Use verbose output
--allow-untracked Allows publishing with untracked and uncommitted files
-q, --quiet No output printed to stdout
--color WHEN Coloring: auto, always, never

Expand All @@ -40,10 +42,11 @@ pub fn execute(options: Options, config: &Config) -> CliResult<Option<()>> {
flag_host: host,
flag_manifest_path,
flag_no_verify: no_verify,
flag_allow_untracked: allow_untracked,
..
} = options;

let root = try!(find_root_manifest_for_wd(flag_manifest_path.clone(), config.cwd()));
try!(ops::publish(&root, config, token, host, !no_verify));
try!(ops::publish(&root, config, token, host, !no_verify, allow_untracked));
Ok(None)
}
42 changes: 41 additions & 1 deletion src/cargo/ops/registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,13 @@ pub fn publish(manifest_path: &Path,
config: &Config,
token: Option<String>,
index: Option<String>,
verify: bool) -> CargoResult<()> {
verify: bool, allow_untracked: bool) -> CargoResult<()> {
let pkg = try!(Package::for_path(&manifest_path, config));

if !allow_untracked {
try!(check_directory_cleanliness());
}

if !pkg.publish() {
bail!("some crates cannot be published.\n\
`{}` is marked as unpublishable", pkg.name());
Expand All @@ -55,6 +59,42 @@ pub fn publish(manifest_path: &Path,
Ok(())
}

fn check_git_cleanliness(repo: &git2::Repository) -> CargoResult<()> {
let mut opts = git2::StatusOptions::new();
opts.include_untracked(true).recurse_untracked_dirs(true);
let status = try!(repo.statuses(Some(&mut opts)));
let files:Vec<String> = status.iter().map(|entry| {
let file = entry.index_to_workdir()
.and_then(|dir| dir.old_file().path());
match file {
Some(file) => format!("{}", file.display()),
None => "file not displayable".to_string()
}
}).collect();

if !files.is_empty(){
bail!("{} uncommited or untacked files \
that need to be addressed before publishing. to force the \
publish command include --allow-untracked\nproblem files:\n{}",
files.len(), files.join("\n"));
}
Copy link
Member

Choose a reason for hiding this comment

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

This can probably just be:

if !files.is_empty() {
    bail!("...");
}
Ok(())


Ok(())
}

fn open_git_repo() -> Result<git2::Repository, git2::Error> {
let current_path = Path::new(".");
git2::Repository::discover(current_path)
Copy link
Member

Choose a reason for hiding this comment

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

This is so small now it can probably just be inlined below:

git2::Repository::discover(".").and_then(check_git_cleanliness)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure how. the lack of a known system to check (git) should return on Ok(()). check_directory_cleanliness logic

if git 
  check git
elsif otherSystem
 check other system
else 
  unsupported system return ok.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, yeah, in that case you could do something like:

match discover(".") {
    Ok(repo) => check(repo),
    Err(..) => Ok(()),
}

}

fn check_directory_cleanliness() -> CargoResult<()> {
if let Ok(repo) = open_git_repo() {
check_git_cleanliness(&repo)
} else {
Ok(())
}
}

fn verify_dependencies(pkg: &Package, registry_src: &SourceId)
-> CargoResult<()> {
for dep in pkg.dependencies().iter() {
Expand Down
12 changes: 9 additions & 3 deletions tests/test_bad_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use support::{project, execs};
use support::{project, execs, paths};
use support::registry::Package;
use support::git::repo;
use hamcrest::assert_that;

fn setup() {}
Expand Down Expand Up @@ -57,7 +58,8 @@ Caused by:
});

test!(bad3 {
let foo = project("foo")
let root = paths::root().join("bad3");
let p = repo(&root)
.file("Cargo.toml", r#"
[package]
name = "foo"
Expand All @@ -69,7 +71,11 @@ test!(bad3 {
[http]
proxy = true
"#);
assert_that(foo.cargo_process("publish").arg("-v"),
p.build();

let mut cargo = ::cargo_process();
cargo.cwd(p.root());
assert_that(cargo.arg("publish").arg("-v"),
execs().with_status(101).with_stderr("\
[ERROR] invalid configuration for key `http.proxy`
expected a string, but found a boolean in [..]config
Expand Down
126 changes: 117 additions & 9 deletions tests/test_cargo_publish.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use flate2::read::GzDecoder;
use tar::Archive;
use url::Url;

use support::{project, execs};
use support::execs;
use support::paths;
use support::git::repo;

Expand Down Expand Up @@ -36,8 +36,100 @@ fn setup() {
.build();
}

test!(uncommited_git_files_allowed {
let root = paths::root().join("uncommited_git_files_allowed");
let p = repo(&root)
.file("Cargo.toml", r#"
[project]
name = "foo"
version = "0.0.1"
authors = []
license = "MIT"
description = "foo"
"#)
.nocommit_file("bad","file")
.file("src/main.rs", "fn main() {}");
p.build();
let mut cargo = ::cargo_process();
cargo.cwd(p.root());
assert_that(cargo.arg("publish").arg("--allow-untracked").arg("--no-verify"),
execs().with_status(0).with_stdout(&format!("\
[UPDATING] registry `{reg}`
[PACKAGING] foo v0.0.1 ({dir})
[UPLOADING] foo v0.0.1 ({dir})",
dir = p.url(),
reg = registry())));
});

test!(uncommited_git_files_error_from_sub_crate {
let root = paths::root().join("sub_uncommited_git");
let p = repo(&root)
.file("Cargo.toml", r#"
[project]
name = "foo"
version = "0.0.1"
authors = []
license = "MIT"
description = "foo"
"#)
.nocommit_file("bad.txt","file")
.file("src/main.rs", "fn main() {}")
.file("sub/lib.rs", "pub fn l() {}")
.file("sub/Cargo.toml", r#"
[package]
name = "crates-io"
version = "0.2.0"
authors = []
license = "MIT/Apache-2.0"
repository = "https://github.com/rust-lang/cargo"
description = """
"""

[lib]
name = "crates_io"
path = "lib.rs"
"#);
p.build();

let mut cargo = ::cargo_process();
cargo.cwd(p.root().join("sub"));
assert_that(cargo.arg("publish").arg("--no-verify"),
execs().with_status(101).with_stderr(&"\
[ERROR] 1 uncommited or untacked files that need to be addressed before \
publishing. to force the publish command include --allow-untracked
problem files:
bad.txt",
));
});

test!(uncommited_git_files_error {
let root = paths::root().join("uncommited_git_files_error");
let p = repo(&root)
.file("Cargo.toml", r#"
[project]
name = "foo"
version = "0.0.1"
authors = []
license = "MIT"
description = "foo"
"#)
.nocommit_file("bad.txt","file")
.file("src/main.rs", "fn main() {}");
p.build();

let mut cargo = ::cargo_process();
cargo.cwd(p.root());
assert_that(cargo.arg("publish").arg("--no-verify"),
execs().with_status(101).with_stderr_contains(&"\
[ERROR] 1 uncommited or untacked files that need to be addressed before \
publishing. to force the publish command include --allow-untracked
problem files:",
).with_stderr_contains(&"bad.txt"));
});

test!(simple {
let p = project("foo")
let root = paths::root().join("simple");
let p = repo(&root)
.file("Cargo.toml", r#"
[project]
name = "foo"
Expand All @@ -47,8 +139,12 @@ test!(simple {
description = "foo"
"#)
.file("src/main.rs", "fn main() {}");
p.build();

let mut cargo = ::cargo_process();
cargo.cwd(p.root());

assert_that(p.cargo_process("publish").arg("--no-verify"),
assert_that(cargo.arg("publish").arg("--no-verify"),
execs().with_status(0).with_stdout(&format!("\
[UPDATING] registry `{reg}`
[PACKAGING] foo v0.0.1 ({dir})
Expand Down Expand Up @@ -84,7 +180,8 @@ test!(simple {
});

test!(git_deps {
let p = project("foo")
let root = paths::root().join("git_deps");
let p = repo(&root)
Copy link
Member

Choose a reason for hiding this comment

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

How come these tests had to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the test would discover the parent git repo and fail to pass because it would be dirty. I made them repos so they have their own git stack and stop there.

Copy link
Member

Choose a reason for hiding this comment

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

Hm ok. This is a little worrisome because I know of some workflows where entire home directories are git repositories which means that Cargo projects not in a git repository may end up producing lots of spurious warnings like this. I guess cargo new creates new git repos though so maybe it's not too bad.

I think though it may be a case where it's easy-ish to handle by just checking to see if Cargo.toml is in version control at all. If you're publishing a crate and we discovered a git repo where the Cargo.toml is explicitly ignored, then you can probably ignore that git repo for status checks. I think that'll take care of these tests at least.

Copy link
Contributor Author

@sbeckeriv sbeckeriv May 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry i hit submit early.

I had some back and forth on this too. There will be errors if someone has

home - git 
-- overall project - hg
---- sub project that is a crate - hg

I think a git in git might be more common. Or a directory for different projects.

- open source project - top git level
-- osx libs 
---- ruby
---- rust
----- crate code

So in this case the root is 4 levels up and covers other projects. In this case we would error if there was uncommitted code in ruby.

You want me to check if the git project ignores the Cargo.toml file? I think at this point letting the user set a config in .cargo/config would be less of a guessing game. It would also inform the tagging part of publishing.

[publish]
version_control=false

When I get a chance I will read up on builder again. Im not sure if they support sub-gems.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the idea here is that we want as few false positives as possible, so relying on configuration to weed them out is unfortunately fairly unergonomic. I think we may want logic like if Cargo.toml is ignored, skip all checks, and otherwise we check the entire repo for cleanliness.

This is somewhat of a prerequisite for tagging releases and such as well as we need to be sure that what we tagged is indeed what was released.

.file("Cargo.toml", r#"
[project]
name = "foo"
Expand All @@ -97,16 +194,20 @@ test!(git_deps {
git = "git://path/to/nowhere"
"#)
.file("src/main.rs", "fn main() {}");
p.build();

assert_that(p.cargo_process("publish").arg("-v").arg("--no-verify"),
let mut cargo = ::cargo_process();
cargo.cwd(p.root());
assert_that(cargo.arg("publish").arg("-v").arg("--no-verify"),
execs().with_status(101).with_stderr("\
[ERROR] all dependencies must come from the same source.
dependency `foo` comes from git://path/to/nowhere instead
"));
});

test!(path_dependency_no_version {
let p = project("foo")
let root = paths::root().join("path_dependency_no_version");
let p = repo(&root)
.file("Cargo.toml", r#"
[project]
name = "foo"
Expand All @@ -126,16 +227,20 @@ test!(path_dependency_no_version {
authors = []
"#)
.file("bar/src/lib.rs", "");
p.build();

assert_that(p.cargo_process("publish"),
let mut cargo = ::cargo_process();
cargo.cwd(p.root());
assert_that(cargo.arg("publish"),
execs().with_status(101).with_stderr("\
[ERROR] all path dependencies must have a version specified when publishing.
dependency `bar` does not specify a version
"));
});

test!(unpublishable_crate {
let p = project("foo")
let root = paths::root().join("unpublishable_crate");
let p = repo(&root)
.file("Cargo.toml", r#"
[project]
name = "foo"
Expand All @@ -146,8 +251,11 @@ test!(unpublishable_crate {
publish = false
"#)
.file("src/main.rs", "fn main() {}");
p.build();

assert_that(p.cargo_process("publish"),
let mut cargo = ::cargo_process();
cargo.cwd(p.root());
assert_that(cargo.arg("publish"),
execs().with_status(101).with_stderr("\
[ERROR] some crates cannot be published.
`foo` is marked as unpublishable
Expand Down
10 changes: 8 additions & 2 deletions tests/test_cargo_registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use std::fs::{self, File};
use std::io::prelude::*;

use support::{project, execs};
use support::git::repo;
use support::paths::{self, CargoPathExt};
use support::registry::{self, Package};
use support::git;
Expand Down Expand Up @@ -550,7 +551,8 @@ test!(login_with_no_cargo_dir {

test!(bad_license_file {
Package::new("foo", "1.0.0").publish();
let p = project("all")
let root = paths::root().join("bad_license_file");
let p = repo(&root)
.file("Cargo.toml", r#"
[project]
name = "foo"
Expand All @@ -563,7 +565,11 @@ test!(bad_license_file {
.file("src/main.rs", r#"
fn main() {}
"#);
assert_that(p.cargo_process("publish").arg("-v"),
p.build();

let mut cargo = ::cargo_process();
cargo.cwd(p.root());
assert_that(cargo.arg("publish").arg("-v"),
execs().with_status(101)
.with_stderr("\
[ERROR] the license file `foo` does not exist"));
Expand Down