search

alexcrichton / tar-rs

Tar file reading/writing for Rust
https://docs.rs/tar
Apache License 2.0
616 stars 178 forks source link

Unify `mtime` constant used on Unix and Windows #346

Closed mkaput closed 1 month ago

mkaput commented 8 months ago

fix #341

mkaput commented 1 month ago

I'm personally tentatively OK with changing this, but can you hoist this to a const ARBITRARY_DETERMINISTIC_TIMESTAMP that is explicitly shared?

Done

We may also want to consider e.g.

#[cfg(windows)]
pub const PREVIOUS_DETERMINISTIC_TIMESTAMP: <old value>
#[cfg(unix)
pub const PREVIOUS_DETERMINISTIC_TIMESTAMP: <current value>

or so so that anyone who is bit by this can explicitly still use HeaderMode::Deterministic but then also override the time back to the previous value?

I feel uneasy with this because:

  1. This makes something that used to be a bug, soon to be fixed, a public API (?) Looks contradictory.
  2. If somebody heavily relied on these values, then this means they made them part of their public interface (i.e. they leaked tar-rs implementation detail as their public thing). So this looks like a problem of the user that tar-rs should not worry about?
  3. This change is worthy of a breaking release anyway, I believe?
  4. This crate also supports other targets (wasm32), for which such constant does not exist.
mkaput commented 1 month ago

For context, that's how I reacted to this problem in the project I stumbled upon this: https://github.com/software-mansion/scarb/blob/fc81b059a81d164c88eaed021a7b84c63fee5d00/scarb/src/ops/package.rs#L456-L466

cgwalters commented 1 month ago

Right. Actually if we were breaking semver I'd argue HeaderMode::Deterministic should become HeaderMode::Deterministic { mtime: u64 } - and callers should pass their own timestamp. This is e.g. what I do in https://github.com/coreos/cargo-vendor-filterer/blob/28c7760484240184c95f76b5a3f11d92a6779ff1/src/main.rs#L492 which ultimately takes the timestamp from the git commit, as is recommended by https://reproducible-builds.org/docs/source-date-epoch/

mkaput commented 1 month ago

Right. Actually if we were breaking semver I'd argue HeaderMode::Deterministic should become HeaderMode::Deterministic { mtime: u64 } - and callers should pass their own timestamp. This is e.g. what I do in https://github.com/coreos/cargo-vendor-filterer/blob/28c7760484240184c95f76b5a3f11d92a6779ff1/src/main.rs#L492 which ultimately takes the timestamp from the git commit, as is recommended by https://reproducible-builds.org/docs/source-date-epoch/

That sounds like the smartest move because it would force people to reason about that timestamp.

alexcrichton commented 1 month ago

Thanks for the PR (and sticking it out through the delay @mkaput) and thanks for the review @cgwalters!