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

do_not_package_if_src_was_modified spurious errors #6717

Closed
ehuss opened this issue Mar 5, 2019 · 4 comments · Fixed by #6740
Closed

do_not_package_if_src_was_modified spurious errors #6717

ehuss opened this issue Mar 5, 2019 · 4 comments · Fixed by #6740
Labels
A-testing-cargo-itself Area: cargo's tests C-bug Category: bug

Comments

@ehuss
Copy link
Contributor

ehuss commented Mar 5, 2019

do_not_package_if_src_was_modified is failing on macos CI with:

Expected: execs
    but: exited with exit code: 0

Cargo is not able to always detect a modification during packaging on low-resolution filesystems like HFS if the package is extracted and build.rs runs within 1 second.

This is caused by #6257. Previously the timestamps of the original files were in the past, but now the timestamps are all set to the extraction time.

Some options to fix:

  1. Change tar to optionally set timestamps, but ignore errors.
  2. Disable do_not_package_if_src_was_modified on macos.
  3. Change run_verify to use a better method for detecting modifications.
  4. Revert Configure tar to not set mtime #6257. Cargo may set mtimes in other places (see Rebuild on mid build file modification #6484).
    a. Note: I cannot reproduce Failure to package on FAT-family filesystems #6238. I tried creating various FAT partitions on Windows 8 and Windows 10, and they all seemed to be able to set file times just fine. How can I repro it?

Any thoughts/preferences?

I get about 20% failure locally. Seen recently at

@ehuss ehuss added C-bug Category: bug A-testing-cargo-itself Area: cargo's tests labels Mar 5, 2019
@dwijnand
Copy link
Member

dwijnand commented Mar 5, 2019

travis-ci/travis-ci#2993 logs available via email request

@alexcrichton
Copy link
Member

I'd be fine with either reverting or ignoring on osx, it's not super critical this test runs everywhere. That we require the ability to set mtime is a good point, although the main tarbal extraction point, $CARGO_HOME, is more likely to be in a "different and weird filesystem" than the build directory which may mean we should just ignore the test.

@ehuss
Copy link
Contributor Author

ehuss commented Mar 6, 2019

Verification is done in target/package, not CARGO_HOME.

@ehuss
Copy link
Contributor Author

ehuss commented Mar 6, 2019

@khionu Can you give me instructions for setting up a filesystem that reproduces #6238? The change from #6257 is causing some CI problems, and I want to better understand what motivated the change. A few months ago, Cargo switched to requiring the ability to set mtimes, so I'm thinking unless there is a strong motivation for such filesystems, reverting #6257 might be the best option. Supporting filesystems that can't adjust mtimes may take a fair bit of effort at this stage. (EDIT: "require" isn't the right word here.)

bors added a commit that referenced this issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing-cargo-itself Area: cargo's tests C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants