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

pragmatically compare timestamps (as suggested by kornelski) #13955

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

gilescope
Copy link
Contributor

@gilescope gilescope commented May 23, 2024

AKA 'make docker great again'.

Fixes #12060 (as coded here: master...kornelski:cargo:nanotime )

Let's cut out needless rebuilds. (Thank you @kornelski for suggesting this)

If we are concerned about backward compatibility, we can put this behind a nightly only flag so this slight behaviour change is not the default.

@rustbot
Copy link
Collaborator

rustbot commented May 23, 2024

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-rebuild-detection Area: rebuild detection and fingerprinting S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2024
@weihanglo weihanglo added the T-cargo Team: Cargo label May 23, 2024
@weihanglo
Copy link
Member

weihanglo commented May 23, 2024

@rfcbot fcp merge

This helps mitigate the needless rebuild when a filesystem supports only low-precision mtime, with a caveat:

there's only one in a billion chance that the nanosecond part will be exactly 0.

I propose to merge without any nightly flag, as the probability is relatively low.

@rfcbot
Copy link
Collaborator

rfcbot commented May 23, 2024

Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels May 23, 2024
@joshtriplett
Copy link
Member

@rfcbot concern one-in-a-billion-chances-happen-at-scale

I'm all for handling timestamp truncation in a reasonable way. But I don't want us making the assumption that one-in-a-billion chances don't happen. If you're running a large fleet of systems doing many many operations per day, one-in-a-billion chances can be a regular occurrence.

Can we guard against this to ensure we only hit this case due to timestamp granularity, and not by random chance?

@ehuss
Copy link
Contributor

ehuss commented May 27, 2024

I don't think 1 billion is an accurate representation of the precision or probability that the sub-second time will be 0. Different systems and filesystems have different precision. For example, NTFS is 100-nanosecond, some virtualization environments or limited hardware might be much less.

I'm not sure why the updated patch here only checks for one file. I would assume if one side or the other is stored on a truncated filesystem, then all files would be truncated, and if not some number from 0 to less than all could possibly be 0.

@gilescope
Copy link
Contributor Author

Doing more experimentation, I've seen drift from 0ns to 14ns on a large codebase.
I'd be inclined to be lenient on the check only if every file is < 25ns (overridable by CARGO_MAX_MTIME_DRIFT_NS)

@bors
Copy link
Contributor

bors commented Oct 8, 2024

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

@kornelski
Copy link
Contributor

I've noticed that the timestamps are wrong only on build_script_build files, so I'm wondering whether there's a different bug in the way build scripts are written or fingerprinted that causes this problem.

@rustbot
Copy link
Collaborator

rustbot commented Dec 20, 2024

☔ The latest upstream changes (possibly 081d7ba) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rebuild-detection Area: rebuild detection and fingerprinting disposition-merge FCP with intent to merge proposed-final-comment-period An FCP proposal has started, but not yet signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cargo Team: Cargo
Projects
Status: FCP blocked
Development

Successfully merging this pull request may close these issues.

Spurious rebuilds due to filesystem rounding file modification timestamps
8 participants