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

Add timestamp macro #9057

Conversation

straight-shoota
Copy link
Member

This adds a new top level macro timestamp which returns the current time as unix timestamp.

The value is identical throughout each invocation inside a program and represents the start of the compilation process.

A use case is in the compiler which currently shells out to get the timestamp for SOURCE_DATE_EPOCH. This is not portable (see #9054 and #9049) and can easily be replaced by a macro feature.

time = {{ (env("SOURCE_DATE_EPOCH") || `date +%s`).to_i }}

@RX14
Copy link
Contributor

RX14 commented Apr 13, 2020

I kind of like the idea of having a method which takes a format string, this seems quite specific. But, it's not terrible.

@j8r
Copy link
Contributor

j8r commented Apr 13, 2020

The idea is good, but the method is not flexible.
There are lots of cases when a date is needed in macros, which is not only the UNIX epoch, like the build time of the program. A examples is shards.

I propose a time(format) macro. A few examples:

  • time("%c") will return the date and time (Mon Apr 13 19:00:00 2020)
  • time("%s") will return the time in seconds since 1970-01-01 00:00:00 UTC (epoch time)

One can say Time.unix({{timestamp}}).to_s("%Y-%m-%d") can be used - {{time("%Y-%m-%d")}} is much nicer.

@asterite
Copy link
Member

I think this macro method is fine. With it you can construct a time instance and format it however you want. There's no need to do the formatting at compile-time.

@j8r
Copy link
Contributor

j8r commented Apr 13, 2020

@asterite the formatting here is already done, it is only UNIX epoch.
Sure it is perfect for this very use case it solves, but for others not.

it has even less sense on non-UNIX platforms.

@jhass
Copy link
Member

jhass commented Apr 13, 2020

What if we have the macro emit a Time instance? So currently emit something like "Time.unix_ms(#{@program.timestamp.to_unix_ms})" but that just being an implementation detail.

@asterite
Copy link
Member

@j8r if you start from a unix timestamp then you can create a Time instance and format however you want. What can't you do with the current approach?

@straight-shoota
Copy link
Member Author

@j8r That's a legitimate idea, but I really don't think this flexibility is necessary. I'd like to keep the macro interface as simple as possible. Formatting can easily be done at runtime, and even more than that. (Just to clarify: Unix timestamps are a commonly used format and obviously work on windows, too)
The same KISS argument goes against @jhass' proposal. The compiler would need to have a hard coded understaning of a non-essential stdlib API.
This is just intended as a simple low level API. It gives you everything you need.
But we don't need a fancy macro API.

@j8r
Copy link
Contributor

j8r commented Apr 13, 2020

@straight-shoota I am also saying, timestamp is not a clear enough method name. We don't know what this number is, and even what is the unit.

Here it is not an "UNIX timestamp", it is a UNIX Epoch time in seconds.

A better name would be unix_time.

@straight-shoota
Copy link
Member Author

The number is the same as what we use for Time.unix and Time#to_unix.

A better name would be unix_time.

I don't think that would be better because then you could legitimately ask "time of what"?
timestamp seems to more clearly communicate that it's the current time.

Maybe an alternative would be to use a constant instead of a method. We could call it BUILD_DATE_EPOCH and it would not only be available at compile time but also in runtime.
That would highlight the fact that it's a single value, defined once in the compiler process.

@Sija
Copy link
Contributor

Sija commented Apr 13, 2020

@straight-shoota Using a constant for that seems to me like a more clear and elegant solution indeed.

@straight-shoota
Copy link
Member Author

Ideally, it should probably go into Crystal namespace but then it somewhat collides with Crystal::BUILD_DATE which contains the compiler's build date...

@j8r
Copy link
Contributor

j8r commented Apr 13, 2020

@straight-shoota please see Unix Time. It is a standard, well known name.
If one does not know it, a quick web search will give the result.
unix_epoch_time is also an alternative.

@j8r
Copy link
Contributor

j8r commented Apr 13, 2020

It also matches the stdlib. There is no Time#to_timestamp, but a Time#to_unix.

@Sija
Copy link
Contributor

Sija commented Apr 13, 2020

@j8r IMHO your explanations seem to completely miss the point that it's the build timestamp, not just a time, whatever the format it may be in. and I'm pretty sure that @straight-shoota knows what the unix time is... xD

@j8r
Copy link
Contributor

j8r commented Apr 13, 2020

@Sija it is both, in fact. The Unix time build date. Univercal Coordinated Time (UTC) build date is also possible is some places.

@asterite
Copy link
Member

Can we pass this value as an environment variable? That way we won't need this at compile time.

@straight-shoota
Copy link
Member Author

@asterite I don't follow. How & when would you pass this as env var? The idea of SOURCE_DATE_EPOCH etc. is to be compiled into the binary, so it needs to be determined at compile time.

@asterite
Copy link
Member

@straight-shoota

SOURCE_DATE_EPOCH=1586895605 crystal some_program.cr
# some_program.cr
COMPILETIME = {{env("SOURCE_DATE_EPOCH")}}

@asterite
Copy link
Member

Then we pass SOURCE_DATE_EPOCH in bin/crystal. Or packages pass this value. They compute the date in the shell. How the date is computed is not Crystal's problem anymore (just like what we did with the compiler version, I think).

@jhass
Copy link
Member

jhass commented Apr 14, 2020

Why would we implement some of the compiler functionality in a wrapper shell script? So far the one shipped by the official packages is just handling environment setup for the choice of those packages to also ship some more libraries. It's perfectly possible to install the crystal binary into /usr/bin if the distro ships those libraries instead, case in point the Archlinux package doing exactly this. Giving this up for a mandatory environment variable that should a different value for each invocation would not be understandable to me. Having this, or rather usages of this, overridable via an environment variable makes sense to facilitate reproducable builds. But making it mandatory, making it impractical to use the compiler binary without a shellscript wrapper, doesn't seem like a wise solution to be honest.

@asterite
Copy link
Member

asterite commented Apr 14, 2020

I think for compiling the rust compiling you need a lot of env variables.

In the case of Ruby, a bot script (matzbot) updates the release time every day, see ruby/ruby@fb40495

I think we are trying to be too smart by moving everything we can into compiler macros, when the plain old solution of getting that information externally so we have a more portable program is probably better.

@straight-shoota
Copy link
Member Author

@asterite I see what you mean and it make sense. I'd fully support your approach if this was only a rare use case for the compiler. But this is useful for other crystal programs as well. So I'm not 100% sold (yet).

But the general idea is quite appealing. In fact, the exact time at which a program was compiled is pretty meaningless. I can take an old source code and old compiler release and compile it now. That doesn't make it a "new" program. It may be years old. Reproducible builds should not care when they happen.

SOURCE_DATE_EPOCH tries to solve that by assigning a meaningful value: the timestamp of the source code. We already support that and the feature we're talking about is essentially just a fallback solution, in case no SOURCE_DATE_EPOCH is available. But the goal should actually be to use a meaningful SOURCE_DATE_EPOCH, not a deficient, autogenerated substitute.

So, what if we fully commit to this way of thinking? I actually don't think it would be a huge issue if we don't provide a default when SOURCE_DATE_EPOCH is missing.
That's just relevant for development builds and those shouldn't care about build date to much anyways. The git ref is more relevant. In case of local builds, you can also look at ctime of the executable to get a time reference.

@asterite
Copy link
Member

Just in case: I'm fine with merging this as is. I think it can be useful to other programs too. I just wonder how many more stuff like this we'll need to move into macros. Maybe this is the last one.

@straight-shoota
Copy link
Member Author

We're actually doing the exact same thing with CRYSTAL_CONFIG_BUILD_COMMIT:

sha = {{ env("CRYSTAL_CONFIG_BUILD_COMMIT") || "" }}

It's just empty when the enviornment variable is missing. No big deal.

If you want that information available, you need to make sure the env variables are defined.

We can iterate on how to improve the tooling. Those values could be pulled from the checked out git commit, for example. I'm not saying the compiler should do this, but it could be some other tool using the compiler and feeding it context information.

@straight-shoota
Copy link
Member Author

Closing in favour of #9088.

@straight-shoota straight-shoota deleted the feature/macro-timestamp branch April 17, 2020 23:32
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.

None yet

7 participants