Make the build reproducible. #153

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@lamby
lamby commented Feb 6, 2017

Whilst working on the Reproducible Builds effort [0], we noticed
that pnmixer could not be built reproducibly as it embeds the current
time in a locale and timezone-varying manner

[0] https://reproducible-builds.org/

Signed-off-by: Chris Lamb chris@chris-lamb.co.uk

@lamby lamby Make the build reproducible.
Whilst working on the Reproducible Builds effort [0], we noticed
that pnmixer could not be built reproducibly as it embeds the current
time in a locale and timezone-varying manner

 [0] https://reproducible-builds.org/

Signed-off-by: Chris Lamb <chris@chris-lamb.co.uk>
5ac0090
@hasufell
Collaborator
hasufell commented Feb 6, 2017 edited

If the value is malformed, the build process SHOULD exit with a non-zero error code.

https://reproducible-builds.org/specs/source-date-epoch/#idp43331376

It doesn't seem we do that or does date already take care of that?

@elboulangero
Collaborator

Anyway, I suggest that we just remove the automatic date from the man page, for two reasons:

  • the date in the man page should be the date of the last modification in the man page, NOT the build date (according to my own logic, feel free to disagree).
  • it's not worth adding complexity to the build just for managing this date in the man page.

So I suggest we hard-code the date, and update it manually when we update the man page. Afraid that we forget ? Then we just remove the date from the manpage, who cares anyway ? We already embed the version, it seems enough to me.

@hasufell
Collaborator
hasufell commented Feb 6, 2017

So I suggest we hard-code the date, and update it manually when we update the man page.

We could add a convenience make target to sed it and document it in HACKING.md or so.

@lamby
lamby commented Feb 6, 2017

I'd personally just drop the date entirely. Nobody really relies on it anymore now that we have pervasive VCS history :)

@hasufell
Collaborator
hasufell commented Feb 6, 2017

Nobody really relies on it anymore now that we have pervasive VCS history

VCS is for development. I don't like to assume that users interact with our development tools, although a lot of them do these days.

It's a trivial decision, but I think it's still unrelated to our development tools. The date is either useful... or not.

@elboulangero
Collaborator

What about setting the date automatically by taking the date of the last commit on the file ?

Upside:

  • it makes the build reproducible.
  • it really represents the last time the man page was edited (which is what we want, see man man-pages).

Downside:

  • it's broken if the source code was not cloned with git (aka taken from a tar archive).
@lamby
lamby commented Feb 11, 2017

You can do that with a nice fallback like SOURCE_DATE_EPOCH=${SOURCE_DATE_EPOCH:-$(git log -1 --pretty=%c)}

@elboulangero
Collaborator

Yep indeed.

Another idea: use a git pre-commit hook. So that the date is not set a build-time, but at commit time.

#!/bin/sh
#
# A git hook to ensure the date in the man page is up to date
#

confirm_commit()
{
        local message="Do you still want to commit ? [y/n] "

        while true; do
                read -p "$message" answer
                case "$answer" in
                        [Yy]|YES|Yes|yes) return 0; break;;
                        [Nn]|NO|No|no)    return 1; break;;
                        *) echo >&2 "Please answer yes or no.";;
                esac
        done
}

# Allows us to read user input below, assigns stdin to keyboard
exec </dev/tty

# Check if manual page was modified
man_page=$(git diff --cached --name-only | grep "pnmixer.0")
if [ -n "$man_page" ]; then
        echo "Hey !"
        echo "It seems that you modified the manual page '$man_page'."
        echo "Can you testify that you updated the date within this file ?"
        confirm_commit || exit 1
fi

# Close stdin
exec <&-

exit 0
@lamby
lamby commented Feb 12, 2017

This is starting to become a little overkill for... a date :)

@elboulangero
Collaborator

Yeah sure :) But on the other hand, there are a few other things that we are prone to forget, so once such a pre-commit hook is setup, it's might be easy to extend it to check for other stuff. Like ensuring, when translations are updated, that credits are updated as well. This kind of things.

However, I'm also not certain that it's worth the hassle of scripting it all like that :)

@hasufell
Collaborator

pre-commit hook

Will that work when I commit via git gui?

Imo, it should be fine if the date is fixed and - in case we are in a git repo - automatically updated on make. Developers will see the change in their git status or gui and the worst thing that can happen is that a user doesn't see the correct date.

@elboulangero
Collaborator

Alright, agreed, here's my first and last attempt. This command-line fiddling exhausted me arleady.

.0.1:
	@sed "s/!VERSION!/@PACKAGE_VERSION@/g" < $*.0 > $@
	@git rev-parse --git-dir >/dev/null 2>&1 && { \
		ts="$$(git log -1 --pretty=%at pnmixer.0)"; \
		month=$$(date --utc --date=@$${ts} '+%B'); \
		year=$$(date --utc --date=@$${ts} '+%Y'); \
		sed -i "s/!DATE!/$$month $$year/g" $@; \
	} || { \
		sed -i "s/!DATE!/January 2017/g" $@; \
	}
	@echo Built $*.1 from template

Please accept or improve, I'm done with that.

@elboulangero
Collaborator

Hmm, well the more I think about it the less I like it.

Because if a distrib compiles PNMixer from git, and another from a tar archive, they might end up having different dates for the man page.

Because if a developer forgets to change the date, the error (because yes it's an error) will go un-noticed.

Actually, I think I prefer the git hook, but a better one that the one I submitted. A git hook that simply checks the date, and fails the commit if the date has not been updated. It will work with git gui, and force the developer to immediately fix his mistake.

Neat and simple.

@hasufell
Collaborator

I think I prefer the git hook

Which a developer might or might not (because he cloned from another location and forgot or whatever) have deployed. So that's not reliable either. And... github doesn't allow for server-side pre-receive hooks. Only the enterprise edition.

Because if a distrib compiles PNMixer from git, and another from a tar archive, they might end up having different dates for the man page.

I don't see what the problem with that is. The main concern is not differing dates between distros, but reproducible builds and for the latter, people will have to use our properly packaged source archives from the release pages.

@hasufell
Collaborator

Please accept or improve, I'm done with that.

I'll propose mine this evening or so.

@elboulangero
Collaborator

Which a developer might or might not (because he cloned from another location and forgot or whatever) have deployed. So that's not reliable either. And... github doesn't allow for server-side pre-receive hooks. Only the enterprise edition.

Oh no no, I don't know what you're talking about ! The git hook is just a file we version, and it's installed by the ./autogen.sh, so it works flawlessly for every dev, as long as they use git.

For example, there are projects like systemd which install the default git hook like that (and by the way it wouldn't hurt to do it as well). See this file, around line 30: https://github.com/systemd/systemd/blob/master/autogen.sh

@elboulangero
Collaborator
elboulangero commented Feb 17, 2017 edited

I think modifying the man page without modifying the date is a mistake. And the very purpose of git pre-commit hooks is to avoid commiting mistakes. Let me quote this page https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks:

The pre-commit hook is run first, before you even type in a commit message. It’s used to inspect the snapshot that’s about to be committed, to see if you’ve forgotten something, to make sure tests run, or to examine whatever you need to inspect in the code.

So in this situation, a pre-commit hook is exactly what we need.

With it, not only we make the build reproducible, but we also improve our development process, and ensure no errors are committed. It's a win-win, a no-brainer ! What reasons would you have to be against git hooks ?

@hasufell
Collaborator

Ok, then let's go for hooks

@elboulangero elboulangero added a commit that referenced this pull request Feb 18, 2017
@elboulangero elboulangero man: hardcode the date #153
This simplifies the build, since now the only variable left in the file
is the package version, and we can expand it at configure time. No need
for a rule in the makefile anymore.
1766776
@elboulangero
Collaborator

Alright I pushed the modifications, it's quite straightforward. Time will tell how much we're happy with it.

@hasufell
Collaborator

Shouldn't we do a point release for that? Maybe include the translation improvements and some other stuff.

@elboulangero
Collaborator

Yep I actually want to do that, along with changes in #152, plus other minor improvements here and there. I had some feedback from a Debian packager, and that's why I'm doing these minor changes lately.

@hasufell hasufell closed this Feb 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment