nicklan / pnmixer

Volume mixer for the system tray
GNU General Public License v3.0
152 stars 32 forks source link

Make the build reproducible. #153

Closed lamby closed 7 years ago

lamby commented 7 years ago

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

hasufell commented 7 years ago

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 commented 7 years ago

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

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 commented 7 years ago

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 commented 7 years ago

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

hasufell commented 7 years ago

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 commented 7 years ago

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

Upside:

Downside:

lamby commented 7 years ago

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

elboulangero commented 7 years ago

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 commented 7 years ago

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

elboulangero commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

I'll propose mine this evening or so.

elboulangero commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Ok, then let's go for hooks

elboulangero commented 7 years ago

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

hasufell commented 7 years ago

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

elboulangero commented 7 years ago

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.