Skip to content

Add option to preserve input tar mtimes in TarFileWriter#11028

Closed
dannysullivan wants to merge 1 commit intobazelbuild:masterfrom
dannysullivan:preserve-tar-mtimes-option
Closed

Add option to preserve input tar mtimes in TarFileWriter#11028
dannysullivan wants to merge 1 commit intobazelbuild:masterfrom
dannysullivan:preserve-tar-mtimes-option

Conversation

@dannysullivan
Copy link
Copy Markdown
Contributor

@dannysullivan dannysullivan commented Mar 26, 2020

Also backfill unit tests of default mtime functionality.

This is the first in a short sequence of changes aimed at fixing static file caching issues in applications deployed using the container_image rule in rules_docker. That rule relies on TarFileWriter, which currently copies files from one tar to another and overwrites the input files' mtimes, which causes the Last-Modified HTTP header to always return the same value and breaks browser caches.

This PR contains an additive change in the form of an optional parameter to TarFileWriter that copies mtimes from the input tar files to the output tar. It should not affect any existing behavior.

@dannysullivan
Add option to preserve input tar mtimes in TarFileWriter
918747f 
Also backfill unit tests of default mtime functionality
@dannysullivan dannysullivan mentioned this pull request Mar 26, 2020
Closed
@dslomov dslomov added team-Rules-Server Issues for serverside rules included with Bazel untriaged labels Apr 3, 2020
@aiuto
Copy link
Copy Markdown

aiuto commented Apr 6, 2020

the tar writing rules within bazelbuild/bazel are deprecated and moved to bazelbuild/rules_pkg, so we won't take the change to this repo.

But there is a fundamentally different problem. The default behavior is a wrong-minded hack.
A tar depending on a tar should, by default, include the tar file as is - not unpack and repack.

We can discuss it that repo, but please as an issue first, instead of a set of PRs.
I don't think preserving mtimes is necessarily the job of the rule itself. It is probably something which should happen as a side effect of --stamp. So the default behavior is a predictable mtime, but the user can get the current mtime with bazel build --stamp //:target

@aiuto aiuto closed this Apr 6, 2020
@dannysullivan dannysullivan mentioned this pull request Apr 7, 2020
Closed
@dannysullivan
Copy link
Copy Markdown
Contributor Author

dannysullivan commented Apr 7, 2020

Thanks for the feedback @aiuto - I just opened the issue here: bazelbuild/rules_pkg#149 . Please let me know if you need any extra information from me!

aiuto
aiuto reviewed Apr 16, 2020
View reviewed changes
@aiuto aiuto reopened this Apr 16, 2020
@aiuto
Copy link
Copy Markdown

aiuto commented Apr 27, 2020

Well.. As much as I want a quick migration to rules_pkg, that may be another 2 or 3 incompatible flag flips in the future. Let's try to merge this to get people over the hump.

@bazel-io bazel-io closed this in 34d67ca Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@aiuto aiuto aiuto approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes team-Rules-Server Issues for serverside rules included with Bazel untriaged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dannysullivan @aiuto @dslomov @googlebot