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

Remove usage of __DATE__ #193

Merged
merged 1 commit into from
Aug 11, 2024
Merged

Conversation

GalaxyShard
Copy link
Contributor

@GalaxyShard GalaxyShard commented Aug 4, 2024

C's __DATE__ prevents reproducible builds[1]; some LLVM frontends enforce build reproducibility (e.g. Zig) and so this does not compile.

__DATE__ is also present in ndstool

@asiekierka
Copy link
Contributor

asiekierka commented Aug 4, 2024

We don't always bump the package version, so we'd need to either use the global BlocksDS version inside tools to replace __DATE__, or specify the build date/commit via the Makefile. The latter solution is probably preferred, as it would also include Git commit information :-) Useful for reproducing things.

@GalaxyShard
Copy link
Contributor Author

GalaxyShard commented Aug 4, 2024

I think embedding the commit might be a bit problematic because if the tool was built just prior to committing then it would have the previous commit, while anyone building from source would have the current commit. The article I linked has a suggestion:

For --version output, if your build is fully deterministic, the hash of the binary itself (and its dynamic library dependencies) can serve as a stable version identifier. You can keep a map of binary hash to all commit hashes that produce that binary somewhere.

It does require reproducible builds to be configured though (if you want it to be consistent for people who build from source), and it may just be simpler to embed the BlocksDS version

Worth noting that it appears only teaktool and ndstool use __DATE__, perhaps the other tools should also have some way to determine the build besides the version number?

@asiekierka
Copy link
Contributor

asiekierka commented Aug 4, 2024

if the tool was built just prior to committing then it would have the previous commit

Both our Docker and package build environment operates after tagging a release on GitHub (in particular, the latter uses the GitHub repositories as a source); historically, we don't get this part wrong, so I think it'd be fine.

if your build is fully deterministic, the hash of the binary itself (and its dynamic library dependencies) can serve as a stable version identifier

It is absolutely not guaranteed to be for regular BlocksDS builds, especially as I think our Docker has a different build environment than our packages. Though it's a good idea to distinguish between the two in case build-specific issues arise, and we generally ship binaries anyway... Hmm.

perhaps the other tools should also have some way to determine the build besides the version number?

I agree.

@AntonioND
Copy link
Member

There's also this variable that we could use, I remember it from some project I worked in that required reproducible builds: https://reproducible-builds.org/docs/source-date-epoch/

@AntonioND
Copy link
Member

In any case, I think updating a version number for every tool is a bad idea. It takes too long to do and it's too easy to forget about one of them. That's also why I don't even bother tagging different versions for all repositories. At the end of the day it's more useful to know "version of grit that comes with BlocksDS 1.1" than "version 2.4.3 of grit".

@asiekierka
Copy link
Contributor

Yes, all of this is why I think it'd be a good idea to unify version numbers to something like grit 2.4.3 (BlocksDS 1.1; abc123d).

@AntonioND
Copy link
Member

I would even remove the first version number. I would keep the BlocksDS version and maybe the commit. This isn't anything radical, it would be formalizing what we have done from the start of BlocksDS. We never update the version number of any repository other than the main one.

@ds-sloth
Copy link
Contributor

ds-sloth commented Aug 4, 2024

I think embedding the commit might be a bit problematic

We also encountered this issue in our game engine TheXTech, and we resolved it by suffixing the embedded commit with -dirty or -d whenever there are uncommitted changes.

@AntonioND
Copy link
Member

Yes, that's how RGBDS does it:

https://github.com/gbdev/rgbds/blame/9a5b3f09027150c71b2a2f3f622f8b285a9b2c56/Makefile#L27

VERSION_STRING	:= `git --git-dir=.git describe --tags --dirty --always 2>/dev/null`

However, if people move on from git, this will become a problem.

@GalaxyShard
Copy link
Contributor Author

I like the idea of using the BlocksDS version plus either the commit hash or the hash of each binary. Should I edit this PR to do one of those? And if so, should I go with the git commit hash or binary hash?

@asiekierka
Copy link
Contributor

@AntonioND Since it's the BlocksDS version in the end, maybe the main Makefile should simply provide a VERSION_STRING= argument to subprojects, with the exception of ones in a separate repo (grit, mmutil, ndstool)? Then we'd at least have one place to change if we "move on from Git".

@AntonioND
Copy link
Member

I actually like that idea. We already do something similar with INSTALLDIR, which is defined in each individual Makefile but it can be set by the parent Makefile.

@AntonioND
Copy link
Member

I've created this issue to keep track of the changes to the version strings: #196

@GalaxyShard GalaxyShard deleted the reproducable-builds branch September 2, 2024 04:27
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.

4 participants