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

Extend pkg_tar rule with stamp_mtime attribute #285

Closed
wants to merge 1 commit into from

Conversation

dennis3620
Copy link

This change extends the pkg_tar rule for stamp support for files' mtime. The attribute stamp_mtime contains a "volatile" workspace status command key which allows to set dynamic mtime for files inside tar archives. The value of the key depends on the workspace status command result but only "volatile" keys are provided as input to pkg_tar rule.

The motivation behind this change relates to #149 and #192. Files are build with Bazel and added as tar archive to container_image rule (rules_docker). These files are served by nginx inside the container. This change should provide proper ETag values for nginx which relies on file's last modification date for caching.

</p>
<p>
<code>
stamp_mtime = "{STAMP_MTIME}",
Copy link
Collaborator

@aiuto aiuto Jan 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this using a different name than BUILD_TIMESTAMP?
But really, this should be a bool. Either stamp the time found in BUILD_TIMESTAMP or use the portable time.

@aiuto
Copy link
Collaborator

aiuto commented Jan 30, 2021

Let's discuss solutions in an issue first. I would like to to have as many people as possible weigh in on what is useful.
There are a lot of questions

  • do we change the stamp of every file?
  • does that include files which came from pre-made archives that have a particular timestamp of their own?
  • some people want to put the timestamp into the output filename. While I find that dubious, any stamping solution should consider it.
  • Why is stamp_mtime an attribute? Why not just support the --stamp flag. That seems to be the API of least surprise.
  • and.... no solutions that just to .tar. We need do do the appropriate thing for each of pkg_zip, pkg_deb and pkg_rpm. But that makes getting the API right before doing any code even more important.

@nacl
Copy link
Collaborator

nacl commented Feb 3, 2021

Design and requirements gathering is tracked in #287.

Copy link
Collaborator

@aiuto aiuto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried my own variant in #288.
It uses a similar technique, but supports --stamp directly, without having to modify the pkg_tar rule.
I prefer that because it feeds into the stamp behavior of the built-in rules like cc_binary. https://docs.bazel.build/versions/master/be/c-cpp.html#cc_binary

Let's finish discussion of what we want in #287 and then see what to implement.

Comment on lines +108 to +110
stamp_mtime_strip = ctx.attr.stamp_mtime.strip()
if not stamp_mtime_strip.startswith("{") or not stamp_mtime_strip.endswith("}"):
fail("You set stamp_mtime, but this doesn't contain a valid key variable")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just use BUILD_STAMP. We don't need the flexibility to let the user set an arbitrary variable out of the volatile status file. This adds complexity that no one asked for.

#!/usr/bin/env bash
stamp_mtime=946684741 # 1999-12-31, 23:58:01
cat << EOF
STAMP_MTIME ${stamp_mtime}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be BUILD_TIMESTAMP

@aiuto
Copy link
Collaborator

aiuto commented May 3, 2021

I'm opting for the path I have taken in #288. That will works the same way as the stamp attribute in the *_binary rules.

@aiuto aiuto closed this May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants