Deterministic time in cache items for reproducible builds #26127
Conversation
| @@ -108,6 +110,11 @@ private function getFile($id, $mkdir = false) | |||
| return $dir.substr($hash, 2, 20); | |||
| } | |||
|
|
|||
| private function getTime() | |||
| { | |||
| return $this->deterministicTime !== null ? $this->deterministicTime : time(); | |||
There was a problem hiding this comment.
Thanks @lstrojny for this PR. I think this is going too far :)
This completely breaks the expiration mechanism of cache pools.
It cannot pass PSR-6 when this flag is on anymore.
Instead, I'd suggest to use a time-less value for non-expirable items.
There was a problem hiding this comment.
On breaking expiration mechanism: doesn't that depend on what I configure as a timestamp. Since I am touching the clock, I might invalidate it too early or too late, but that's my choice as a user.
On PSR-6: can you elaborate where the PSR-6 breakage comes from?
The breakage comes from the first sentence, which is breaking basic expectations from the consumer pov (the instantiation is not the responsibility of the consumer, so it doesn't know that this pool is behaving specially - and doesn't expect it more importantly.) As noted before, I'd suggest to use a time-less value for non-expirable items. |
|
@nicolas-grekas I don't think I agree that this would be so bad but I like your suggestion more, simply because it is way less code and we can live without adding any extra parameter to the classes. |
|
|
| @@ -36,7 +36,7 @@ public function prune() | |||
| continue; | |||
| } | |||
|
|
|||
| if ($time >= (int) $expiresAt = fgets($h)) { | |||
| if (0 < ($expiresAt = fgets($h)) && $time >= $expiresAt) { | |||
There was a problem hiding this comment.
if (($expiresAt = (int) fgets($h)) && $time >= $expiresAt) {
the isset($expiresAt[0]) below looks unneeded now, it's always true now
(same comments on the second change)
| @@ -94,7 +94,7 @@ protected function doHave($id) | |||
| protected function doSave(array $values, $lifetime) | |||
| { | |||
| $ok = true; | |||
| $expiresAt = time() + ($lifetime ?: 31557600); // 31557600s = 1 year | |||
| $expiresAt = $lifetime > 0 ? (time() + $lifetime) : 0; | |||
There was a problem hiding this comment.
I'd suggest to write this as $lifetime ? time() + $lifetime : 0;
|
Thank you @lstrojny. |
…(lstrojny) This PR was merged into the 3.4 branch. Discussion ---------- Deterministic time in cache items for reproducible builds | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Allows setting a deterministic time for cache entries. This can be used to seed builds with a deterministic timestamp for reproducibility. Parent issue is #25958 Commits ------- b4259a6 Use 0 for unlimited expiry
|
Is it expected that when From what i can tell the expiration timestamp is being read from the file itself, so probably just need to add |
|
Please don't comment on old PRs/issues if you want any follow up. |
|
Open an issue or better: a PR instead. |
…y do not expire (Blacksmoke16) This PR was squashed before being merged into the 4.4 branch. Discussion ---------- [Cache] Set mtime of cache files 1 year into future if they do not expire | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? |no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - #26127 made is so that `0` is used to represent cache files that do not expire. However this was causing `touch` to be called with `0`, which was setting the create/modification time of the file to start of epoch time, which doesn't really make sense. It can cause some issues with `tar` for example as when timezones are taken into consideration you end up with warnings like: > tar: app/cache/prod/annotations/@/+/3/cdcAWwqaPORAi0TfsO5Q: implausibly old time stamp 1969-12-31 19:00:00 Given the expiration of the files is stored within the file itself, it's probably safe to not `touch` it if that value is `0`. However is there actually a reason to touch it at all as a file that expires in 6 hours would show as created 6 hours into the future. I also wasn't sure how to test this, so open to suggestions on that/if we need the `touch` at all. Commits ------- 57cad6f [Cache] Set mtime of cache files 1 year into future if they do not expire
…y do not expire (Blacksmoke16) This PR was squashed before being merged into the 4.4 branch. Discussion ---------- [Cache] Set mtime of cache files 1 year into future if they do not expire | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? |no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - symfony/symfony#26127 made is so that `0` is used to represent cache files that do not expire. However this was causing `touch` to be called with `0`, which was setting the create/modification time of the file to start of epoch time, which doesn't really make sense. It can cause some issues with `tar` for example as when timezones are taken into consideration you end up with warnings like: > tar: app/cache/prod/annotations/@/+/3/cdcAWwqaPORAi0TfsO5Q: implausibly old time stamp 1969-12-31 19:00:00 Given the expiration of the files is stored within the file itself, it's probably safe to not `touch` it if that value is `0`. However is there actually a reason to touch it at all as a file that expires in 6 hours would show as created 6 hours into the future. I also wasn't sure how to test this, so open to suggestions on that/if we need the `touch` at all. Commits ------- 57cad6f9a4 [Cache] Set mtime of cache files 1 year into future if they do not expire
Allows setting a deterministic time for cache entries. This can be used to seed builds with a deterministic timestamp for reproducibility.
Parent issue is #25958