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

build: depend on standard variable to let the user define the builddate #7186

Merged
merged 1 commit into from
Dec 17, 2018

Conversation

eli-schwartz
Copy link
Contributor

SOURCE_DATE_EPOCH is the standard for reproducible builds; using it means most groups that care will automatically benefit from this without setting project-specific variables.

https://reproducible-builds.org/docs/source-date-epoch/

...

I am going with the flow here since this currently uses a subprocess, but surely there must be some part of the language spec that allows you to do this without such hacks? Anyway, that's not strictly related to this fix...

@eli-schwartz
Copy link
Contributor Author

Currently Arch Linux has an unreproducible package due to this -- the time-limited results can be seen here: https://tests.reproducible-builds.org/archlinux/community/crystal/crystal-0.27.0-1-x86_64.pkg.tar.xz.html

This seemed like a better fix than adding special code to our package.

@straight-shoota
Copy link
Member

Well, it's always possible to simply set CRYSTAL_CONFIG_BUILD_DATE in the build instructions.

But changing to a standardized environment variable doesn't sound like a bad idea.

The implementation isn't portable, though. These date arguments don't work on BSD.
Maybe it's better to just query the env var and time in a macro and put the logic in Crystal code.

@straight-shoota
Copy link
Member

straight-shoota commented Dec 14, 2018

A disadvantage of SOURCE_DATE_EPOCH vs CRYSTAL_CONFIG_BUILD_DATE is that it's not a human readable format and thus requires processing in the compiler. The content of CRYSTAL_CONFIG_BUILD_DATE instead is simply forwarded as string.

However, it is more accurate as it enforces a granularity of one second. I'm not sure if that is truly relevant, though.

@j8r
Copy link
Contributor

j8r commented Dec 14, 2018

At least I agree on the --utc - the date should be in UTC.

@eli-schwartz
Copy link
Contributor Author

The implementation isn't portable, though. These date arguments don't work on BSD.

Oof, should have thought of that. Actually, the reproducible-builds docs specifically suggest for BSD compatibility to use date -u -d instead of date --utc --date, does that work instead?

@eli-schwartz
Copy link
Contributor Author

A disadvantage of SOURCE_DATE_EPOCH vs CRYSTAL_CONFIG_BUILD_DATE is that it's not a human readable format and thus requires processing in the compiler.

The only people using it (for deterministic builds) don't typically worry about human readability, they worry about accuracy. :)

And, you can easily generate it yourself with date +%s

@straight-shoota
Copy link
Member

It might work for now. Windows will need a different implementation, though. But I guess that can be solved later.

The only people using it (for deterministic builds) don't typically worry about human readability, they worry about accuracy. :)

I meant that CRYSTAL_CONFIG_BUILD_DATE is simply used as string and doesn't add any complexity (such as shell commands with portability issues) to the compiler.
The format doesn't matter, it isn't interpreted. So it can actually be set to a datetime representation with any granularity.

Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

I think replacing CRYSTAL_CONFIG_BUILD_DATE is probably fine. If anyone objects though, i'm fine with continuing to support both.

@straight-shoota
Copy link
Member

straight-shoota commented Dec 15, 2018

i'm fine with continuing to support both.

Having both would probably be the worst option. Either one of them is fine.

src/compiler/crystal/config.cr Outdated Show resolved Hide resolved
@eli-schwartz
Copy link
Contributor Author

I don't know crystal, so I'm somewhat limited to guessing based on generic programming patterns and sticking things in willy-nilly if they look useful. And, it was a lot easier to try to stick with existing subprocesses but change the shell code used, than to try to do real programming in crystal. :D But apparently that won't be so simple...

So looking through google to find out how to manipulate time natively gets me to references for Time.epoch(). Putting that into crystal eval, which seems to be how to run snippets of crystal code...

$ echo 'print Time.epoch(0).to_s("%Y-%m-%d")' | crystal eval
Error in line 1: undefined method 'epoch' for Time.class

Oookay. So, turns out the top google hit for crystal documentation is a very old version, and there is no indication of that on https://crystal-lang.org/api/0.20.0/Time.html (or header dropdowns offering to steer me to the latest version).

I guess, it looks like Time.unix(0) works fine instead. Okay, playing around I get:

ENV.fetch("SOURCE_DATE_EPOCH", Time.now.to_unix().to_s) which seems to work. But not so much in the config.cr file, because undefined macro variable 'time' if I try to put it in the {{ }} and if I don't put it in there, then it always evaluates to the current time. Guessing that this is what makes it evaluated at compile time; does this not accept regular code? If not, where should this go instead?

This would surely be the cleanest way forward, since crystal can control its own API for embedding a number and turning it into a formatted date string. So... how to do it?

@asterite
Copy link
Member

We can probably do this:

def self.date
  unix = {{(env("SOURCE_DATE_EPOCH") || `date +%s`).to_i}}
  Time.unix(unix).to_s("%Y-%m-%d")
end

You can only execute a subset of the language in macros in Crystal so you can't manipulate time and so on. However, we can compute the unix seconds, either with the env var or by using date, then convert that to an integer at compile-time (you can do that), and then stick that value into the program. Then at runtime we create a time instance and format it using regular Crystal code.

Eventually we could introduce one or all of __TIME__, __DATE__, __TIMESTAMP__ that D has, or just a macro method to fetch the current time unix seconds or similar. I'm sure it's useful for any program to know the compile date without resorting to something external (like a Makefile).

@asterite
Copy link
Member

And we should also keep CRYSTAL_CONFIG_BUILD_DATE around, I don't know if any package maintainer uses it.

@RX14
Copy link
Contributor

RX14 commented Dec 16, 2018

I don't know if any package maintainer uses it.

Surely that's a reason not to keep it around? The build date reported by crystal --version should be in a standard machine-readable format. This PR ensures that.

@eli-schwartz
Copy link
Contributor Author

Okay, new version still does the compile-time subprocess thing but embeds an epoch, and then formats that as output. It successfully compiles and respects SOURCE_DATE_EPOCH on my Linux buildserver.

@straight-shoota
Copy link
Member

I don't know if any package maintainer uses it.

Nix pkg uses it. . But that should't be a big deal to change.

Keeping two environment variables for the same purpose is just going to be a burdon. CRYSTAL_CONFIG_BUILD_DATE was only introduced in 0.27.0 and isn't even documented anywhere. It should be perfectly fine to replace it.

@eli-schwartz
Copy link
Contributor Author

Nix pkg uses it. . But that should't be a big deal to change.

I believe Nixos has a global setup hook which sets SOURCE_DATE_EPOCH for all packages they build. This change will let them drop code and rely on their infrastructure defaults. https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/setup-hooks/set-source-date-epoch-to-latest.sh

@asterite
Copy link
Member

I see no problem keeping two environment variables. It's more flexible and maintaining that is just one line of code. But if you all free it's too much we can remove it.

SOURCE_DATE_EPOCH is the standard for reproducible builds; using it
means most groups that care will automatically benefit from this without
setting project-specific variables.

https://reproducible-builds.org/docs/source-date-epoch/
@eli-schwartz
Copy link
Contributor Author

eli-schwartz commented Dec 16, 2018

@peterhoeg relevant to #6788 (comment) I think this is a better solution. Nixos should be fine with this, correct?

To have a more deterministic build. On NixOS we generally set the date in the build environment to the start of the unix epoch.

I guess that would be this instead... https://github.com/NixOS/nixpkgs/blob/6a2c004d0f044b1055e4e6197dfd0db86caa0398/pkgs/stdenv/generic/setup.sh#L244

@RX14
Copy link
Contributor

RX14 commented Dec 17, 2018

There's also NixOS/nixpkgs@81e530a#diff-6b4b935e9a925c39793b071281c8c98a

@asterite I don't see the benefit in keeping two environment variables. It's just more things that a distro maintainer can get wrong (imagine if something starts parsing crystal --version then a distro maintainer changes the date format). Let's keep it simple then revert if we get a real usecase.

@peterhoeg
Copy link
Contributor

Using SOURCE_DATE_EPOCH is definitely better and I will update the package in nix accordingly when this PR hits a release.

@RX14 RX14 added this to the 0.27.1 milestone Dec 17, 2018
@RX14 RX14 merged commit 954c960 into crystal-lang:master Dec 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants