Add message about non-release mode to `crystal --version` #13254

will · 2023-03-31T14:28:09Z

Allow passing in git sha and date to Makefile
Remove empty parens when date is ""
Add message when compiler was not compiled in release mode

When building in a nix derivation, there is no git directory, so you have to pass in the sha manually. Also, all the file modification times are set to epcoh, and I think it'd be nicer to just have the builds not have a date rather than the obviously wrong 1970-01-01.

I don't think we need to always have release mode information, but it would be useful I think to have a way to see when you're working with a non-release compiler.

* Allow passing in git sha and date to Makefile * Remove empty parens when date is "" * Add message when compiler was not compiled in release mode When building in a nix derivation, there is no git directory, so you have to pass in the sha manually. Also, all the file modification times are set to epcoh, and I think it'd be nicer to just have the builds not have a date rather than the obviously wrong 1970-01-01. I don't think we need to always have release mode information, but it would be useful I think to have a way to see when you're working with a non-release compiler.

src/compiler/crystal/config.cr

HertzDevil · 2023-03-31T14:45:08Z

src/compiler/crystal/config.cr

      <<-DOC
-        Crystal #{version} #{formatted_sha}(#{date})
+        Crystal #{version} #{formatted_sha}#{formatted_date}


If a date isn't available, the description would have a trailing space on this line

Good catch, I updated it to use Enumerable#join

src/compiler/crystal/config.cr

straight-shoota · 2023-03-31T15:13:05Z

I would prefer to only add a note about release mode when the compiler is not built in release mode.
Most regular users have a production compiler in release mode and shouldn't have to worry about that.

Such a note is helpful if you use a development build of the compiler, so it's sufficient to to mention in that case.

will · 2023-03-31T15:14:43Z

@straight-shoota that was my initial thought as well and it was that way with the first patch, before I applied @Sija's suggestions. I can put it back to only when not release.

straight-shoota · 2023-03-31T15:15:36Z

Maybe make it a normal sentence, though? "The compiler was not built in release mode."

will · 2023-03-31T15:32:14Z

Ok I put it back to only in development mode and with a message. I figured an extra empty line makes it look nicer than with the two things with labels and colons.

bin/crystal --version
Using compiled compiler at .build/crystal
Crystal 1.8.0-dev [cafe86ee6]

LLVM: 15.0.7
Default target: aarch64-apple-darwin22.3.0

The compiler was not built in release mode

Also fixed the extra trailing space by using join.

I realized that I should have done the first review patch as a git fixup, so I fixed that with a force push, but it's otherwise the same commit to make it easier to track what changed. Then the second review fixes as a second fixup commit. Sorry about the force push, but I think it would have been more weird to have one review patch as a fixup and the other not.

straight-shoota · 2023-03-31T15:39:17Z

src/compiler/crystal/config.cr

+      details = [version]
+      details << "[#{build_commit}]" if build_commit
+      details << "(#{date})" unless date.empty?


Using an array + join is fine, we can merge it with that. But it's suboptimal.
A better way would be what we had before, just with leading whitespace added to each optional piece. Or String.build could also be an alternative.
This is an optional suggestion, my approval doesn't depend on it.

I updated this with String.build just now

Sija · 2023-03-31T15:58:05Z

Maybe make it a normal sentence, though? "The compiler was not built in release mode."

Using a sentence would make it harder to parse by a 3rd party tools.

HertzDevil · 2023-03-31T16:25:12Z

crystal --version is in general not meant to be parsed other than that it begins with Crystal x.y.z. If a tool needs to query whether a Crystal compiler was built in release mode or not, it should do so via crystal env or a suitable crystal eval on the predefined constants under the Crystal namespace.

On a related note, IMO Crystal programs themselves should be able to know whether they are compiled under a release build of the compiler via {{ host_flag?(:release) }}. Currently host_flag contains only flags related to the host target triple though.

Sija · 2023-03-31T17:30:53Z

In my case (raven.cr) I need to query these information from within the Crystal program, so using CLI is not an option.

straight-shoota · 2023-03-31T17:35:12Z

I don't think it makes any difference for machine readability if you grep the version config for the compiler was not built in release mode or release mode: false.
Exposing this information internally to the program is a different story, let's start a new feature discussion about this.. It's definitely available at compile time via {{ flag?(:release) }}, though.

HertzDevil · 2023-03-31T18:23:42Z

{{ flag?(:release) }} returns whether the current program is to be compiled in release mode, not whether the current compiler was compiled in release mode.

HertzDevil · 2023-04-04T13:13:29Z

crystal --version is in general not meant to be parsed other than that it begins with Crystal x.y.z.

The compiler spec failure on CI proves my point, as the failing spec expects that the last line of crystal --version's output contains the host target. (That spec should really be rewritten as part of #13156 though)

straight-shoota

I just have a minor suggestion to reorder the output. But apart from that I'm good.

straight-shoota · 2023-04-07T09:26:11Z

src/compiler/crystal/config.cr

+        io << "\n\nLLVM: " << llvm_version
+        io << "\nDefault target: " << host_target
+
+        io << "\n\nThe compiler was not built in release mode." unless release_mode?


In light of #13254 (comment) I would suggest to put this not in the last line. I think it also makes more sense semantically as a second line, after the compiler build info, and before the LLVM version.

What if someone else tries to get the host target from Crystal::DESCRIPTION.lines[3] instead? You cannot avoid a breakage either way so I wouldn't worry about which line the new text shows up on

Yeah, it's not a strong improvement (although I would argue that lines[-1] is much more obvious than lines[3]).
But I also think it makes more sense semantically after the build info.

Sija reviewed Mar 31, 2023

View reviewed changes

src/compiler/crystal/config.cr Outdated Show resolved Hide resolved

src/compiler/crystal/config.cr Outdated Show resolved Hide resolved

src/compiler/crystal/config.cr Outdated Show resolved Hide resolved

HertzDevil added kind:feature topic:compiler labels Mar 31, 2023

HertzDevil reviewed Mar 31, 2023

View reviewed changes

Sija reviewed Mar 31, 2023

View reviewed changes

src/compiler/crystal/config.cr Outdated Show resolved Hide resolved

src/compiler/crystal/config.cr Outdated Show resolved Hide resolved

will added 2 commits March 31, 2023 17:27

fixup! Add more customizations to Crystal::Config.description

a0b4f18

fixup! Add more customizations to Crystal::Config.description

710f87c

will force-pushed the crystal-config branch from cafe86e to 710f87c Compare March 31, 2023 15:28

straight-shoota approved these changes Mar 31, 2023

View reviewed changes

fixup! Add more customizations to Crystal::Config.description

4b672b6

straight-shoota approved these changes Apr 7, 2023

View reviewed changes

fixup! Add more customizations to Crystal::Config.description

54b1d85

straight-shoota approved these changes Apr 7, 2023

View reviewed changes

straight-shoota added this to the 1.9.0 milestone Apr 9, 2023

straight-shoota removed this from the 1.9.0 milestone Apr 18, 2023

beta-ziliani approved these changes Apr 21, 2023

View reviewed changes

beta-ziliani added this to the 1.9.0 milestone Apr 21, 2023

straight-shoota changed the title ~~Add more customizations to Crystal::Config.description~~ Add message about non-release mode to Crystal::Config.description Apr 21, 2023

straight-shoota changed the title ~~Add message about non-release mode to Crystal::Config.description~~ Add message about non-release mode to crystal --version Apr 21, 2023

straight-shoota merged commit 93f8fd0 into crystal-lang:master Apr 24, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add message about non-release mode to `crystal --version` #13254

Add message about non-release mode to `crystal --version` #13254

will commented Mar 31, 2023

HertzDevil Mar 31, 2023

will Mar 31, 2023

straight-shoota commented Mar 31, 2023

will commented Mar 31, 2023

straight-shoota commented Mar 31, 2023

will commented Mar 31, 2023 •

edited

Loading

straight-shoota Mar 31, 2023

will Apr 7, 2023

Sija commented Mar 31, 2023

HertzDevil commented Mar 31, 2023

Sija commented Mar 31, 2023

straight-shoota commented Mar 31, 2023

HertzDevil commented Mar 31, 2023

HertzDevil commented Apr 4, 2023 •

edited

Loading

straight-shoota left a comment

straight-shoota Apr 7, 2023

HertzDevil Apr 7, 2023

straight-shoota Apr 7, 2023

Add message about non-release mode to crystal --version #13254

Add message about non-release mode to crystal --version #13254

Conversation

will commented Mar 31, 2023

HertzDevil Mar 31, 2023

Choose a reason for hiding this comment

will Mar 31, 2023

Choose a reason for hiding this comment

straight-shoota commented Mar 31, 2023

will commented Mar 31, 2023

straight-shoota commented Mar 31, 2023

will commented Mar 31, 2023 • edited Loading

straight-shoota Mar 31, 2023

Choose a reason for hiding this comment

will Apr 7, 2023

Choose a reason for hiding this comment

Sija commented Mar 31, 2023

HertzDevil commented Mar 31, 2023

Sija commented Mar 31, 2023

straight-shoota commented Mar 31, 2023

HertzDevil commented Mar 31, 2023

HertzDevil commented Apr 4, 2023 • edited Loading

straight-shoota left a comment

Choose a reason for hiding this comment

straight-shoota Apr 7, 2023

Choose a reason for hiding this comment

HertzDevil Apr 7, 2023

Choose a reason for hiding this comment

straight-shoota Apr 7, 2023

Choose a reason for hiding this comment

Add message about non-release mode to `crystal --version` #13254

Add message about non-release mode to `crystal --version` #13254

will commented Mar 31, 2023 •

edited

Loading

HertzDevil commented Apr 4, 2023 •

edited

Loading