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

Stricter package change detection. #6740

Merged
merged 1 commit into from
Mar 13, 2019

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Mar 13, 2019

This changes it so that when cargo package verifies that nothing has modified any source files that it hashes the files' contents instead of checking for mtime changes. mtimes are not always reliable.

This also changes it so that it checks all files in the package file. Previously it skipped files based on search rules such as the exclude list.

Closes #6717

@rust-highfive
Copy link

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

@ehuss
Copy link
Contributor Author

ehuss commented Mar 13, 2019

This is just a proposal of one way to fix the issue.

@alexcrichton
Copy link
Member

I think I might not necessarily know how this connects to #6717 but regardless of that seems like a fine change to me irrespective of that! Do you think though we should consider some other solutions for the issue?

@ehuss
Copy link
Contributor Author

ehuss commented Mar 13, 2019

#6717 is caused by run_verify extracting files without mtime. If build.rs runs in under 1 second, then the file it generates (on HFS) will have an mtime equal to the newest file, so it doesn't believe anything has changed, and the do_not_package_if_src_was_modified test fails to detect a change.

This change addresses it by no longer checking mtimes, but instead file contents.

Some alternate solutions are:

  1. Disable do_not_package_if_src_was_modified on coarse_mtime. This is the simplest and least risky change. I don't think run_verify needs to be perfect, considering you can easily circumvent it with --no-verify.
  2. Change tar to preserve mtimes. I can't find a way to repro the original issue, but presumably it exists somewhere, so I'm reluctant to revert it.
  3. Change tar to preserve mtimes, but ignore errors if it fails. I'm reluctant to bother with this.
  4. Change last_modified_file to return a list of last modified files (or some variation of that).
  5. This PR. This is probably the most risky (unintended breakage).

I don't feel strongly about any option. Any preference? I'm fine with doing alternate PRs.

@alexcrichton
Copy link
Member

Oh ok I think I see what's happening now, I originally though that this was all related to tar and extraction into $CARGO_HOME, but it sounds like this test is just extration with tar and blatting the output into the target directory.

In general we want to more to a more content-based mtime solution but have been hesitant to do so due to performance concerns, but this is not on a performance critical path because it's only done during packaging.

In light of all that I'm personally in favor of the solution proposed here! While agreed that there's always a risk of unintended breakage this looks simple enough that I think we can deal with any breakage as it comes up

@bors: r+

@bors
Copy link
Contributor

bors commented Mar 13, 2019

📌 Commit 78a60bc has been approved by alexcrichton

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 13, 2019
@bors
Copy link
Contributor

bors commented Mar 13, 2019

⌛ Testing commit 78a60bc with merge 9eeece1...

bors added a commit that referenced this pull request Mar 13, 2019
Stricter package change detection.

This changes it so that when `cargo package` verifies that nothing has modified any source files that it hashes the files' contents instead of checking for mtime changes.  mtimes are not always reliable.

This also changes it so that it checks *all* files in the package file. Previously it skipped files based on search rules such as the `exclude` list.

Closes #6717
@bors
Copy link
Contributor

bors commented Mar 13, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing 9eeece1 to master...

@bors bors merged commit 78a60bc into rust-lang:master Mar 13, 2019
bors added a commit to rust-lang/rust that referenced this pull request Mar 30, 2019
Update cargo

Update cargo

22 commits in 0e35bd8af0ec72d3225c4819b330b94628f0e9d0..63231f438a2b5b84ccf319a5de22343ee0316323
2019-03-13 06:52:51 +0000 to 2019-03-27 12:26:45 +0000
- Code cleanup (rust-lang/cargo#6787)
- Add cargo:rustc-link-arg to pass custom linker arguments (rust-lang/cargo#6298)
- Testsuite: remove some unnecessary is_nightly checks. (rust-lang/cargo#6786)
- cargo metadata: Don't show `null` deps. (rust-lang/cargo#6534)
- Some fingerprint cleanup. (rust-lang/cargo#6785)
- Fix fingerprint for canceled build script. (rust-lang/cargo#6782)
- Canonicalize default target if it ends with `.json` (rust-lang/cargo#6778)
- Fix setting `panic=unwind` compiling lib a extra time. (rust-lang/cargo#6781)
- Always nicely show errors from crates.io if possible (rust-lang/cargo#6771)
- Testsuite: Make `cwd()` relative to project root. (rust-lang/cargo#6768)
- Allow `cargo fix` if gitignore matches root working dir. (rust-lang/cargo#6767)
- Remove redundant imports (rust-lang/cargo#6763)
- Handle backcompat hazard with `toml` crate (rust-lang/cargo#6761)
- Fix spurious error in dirty_both_lib_and_test. (rust-lang/cargo#6756)
- Update toml requirement from 0.4.2 to 0.5.0 (rust-lang/cargo#6760)
- Reuse std::env::consts::EXE_SUFFIX (rust-lang/cargo#6758)
- Proptest 0.9.1 (rust-lang/cargo#6753)
- Don't need extern crate in 2018 (rust-lang/cargo#6752)
- Release a jobserver token while locking a file (rust-lang/cargo#6748)
- Minor doc fix for publish command synopsis (rust-lang/cargo#6749)
- Stricter package change detection. (rust-lang/cargo#6740)
- Fix resolving yanked crates when using a local registry. (rust-lang/cargo#6742)
@ehuss ehuss added this to the 1.35.0 milestone Feb 6, 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.

do_not_package_if_src_was_modified spurious errors
5 participants