Add VERSION file #6966

bcardiff · 2018-10-20T00:36:05Z

So as an alternative to #6964. This PR moves to a plain VERSION file that is embedded at compile time. If git information is available the sha is present as before, but is not part of version number.

The version could have 0.26.2-dev the only important thing is to be greater than latest. But since we know the next version is 0.27.0 we can use 0.27.0 (nb: 0.27.0-dev < 0.27.0)

Closes #6964. Fixes #6715. Fixes #4642. Closes #4687.

Update:

Also a CRYSTAL_CONFIG_BUILD_COMMIT env var is used to specify the build commit. The Makefile grab this information from git if available.

Closes #6748.

Sija · 2018-10-20T01:20:06Z

src/compiler/crystal/config.cr

-      # Set explicitly: 0.0.0, ci, HEAD, whatever
-      config_version = {{env("CRYSTAL_CONFIG_VERSION")}}
-      return {config_version, nil} if config_version
+      config_version = {{ `cat #{__DIR__}/../../../VERSION`.stringify.chomp }}


Seeing call to cat, used again in the codebase (windows support coming, ekhem) made me open #6967

Even if a macro is added, we can't use it until the next release. Until then an ifdef/type would be needed.

asterite · 2018-10-20T02:24:56Z

why not a constant in crystal source?

bcardiff · 2018-10-20T02:44:16Z

It’s a nice asset to have the version in a plain file i think. Easier to change (eventually by a bot if you want ;-) )

straight-shoota · 2018-10-20T07:43:40Z

I'd also like a way to specify the commit SHA explicitly, either in VERSION or an env var. Development builds shouldn't depend on using git on the build system. That's relevant for example when package managers provide edge builds. That could be added separately, though.

IMHO it would also be better to move the git logic out of the compiler, and just provide a value by the build system like in #6748. It feels much cleaner to not have this hard coded into the compiler.

And I would suggest to add a sanity check to make sure the version is a valid semver version compatible with compare_versions. Otherwise the compiler build would be unusable.

straight-shoota · 2018-10-20T07:51:07Z

src/compiler/crystal/config.cr

+                       {% else %}
+                       {{ `cat #{__DIR__}/../../../VERSION`.stringify.chomp }}
+                       {% end %}
+      git_describe = {{ `(git describe --tags --long --always 2>/dev/null) || true`.stringify.chomp }}


git rev-parse --short HEAD 2> /dev/null returns the sha directly. No need to parse the describe string.

RX14 · 2018-10-20T09:35:30Z

perhaps it would also be a good idea to add a CI check to fail the build if we're building a tag and VERSION isn't the same as the tag name.

oprypin · 2018-10-20T17:09:36Z

Well, at that point it's too late anyway... Need some kind of preliminary check

RX14 · 2018-10-21T13:16:56Z

@oprypin it's not too late to release another version bump at that point. If you're following the release checklist, it will "never happen", but I'd rather have more layers of safety than are required.

straight-shoota · 2018-10-21T13:37:27Z

If you run the specs locally before pushing the release, it's not even to late.

straight-shoota · 2018-10-22T15:07:47Z

src/compiler/crystal/config.cr

-      tag = "#{tag}+#{commits}" unless commits == "0" # Reappend commits since release unless we hit it exactly
-
-      {tag, sha}
+      {config_version, sha}


I suppose this could be separated into two methods now. Unless we'd want to consider the option of specifying the sha in the VERSION file as well, there is no longer any shared logic between "computing" version and sha.

bcardiff · 2018-10-22T15:20:17Z

@straight-shoota @RX14 I applied all the suggestions so far.

straight-shoota · 2018-10-22T15:37:56Z

What do you think about specifying the SHA as an environment variable? It should at least be optional (when not building in a git environment). I'd even prefer it as the only means and provide the value from the Makefile/build script. It would be cleaner not to call git from the compiler.

bcardiff · 2018-10-22T16:43:19Z

@straight-shoota ok. it will require some changes in the distribution scripts doing this (and I am a bit reluctant usually to do it)

A CRYSTAL_CONFIG_SHA env variable is used to grab the information after the last commit. If empty, then no sha is shown.

j8r · 2018-10-22T16:57:16Z

CRYSTAL_CONFIG_SHA isn't explicit enough - perhaps CRYSTAL_COMMIT_HASH

bcardiff · 2018-10-22T17:05:40Z

@j8r It matches the method name Crystal::Config.sha

j8r · 2018-10-22T17:56:17Z

That's fair. The point is the method/variable name don't tell what they are – what is a "Crystal config sha"? The method can be renamed to something from Crystal::Config.sha to something like Crystal::Config.commit_hash?

bcardiff · 2018-10-22T18:03:18Z

I wanted to avoid changing api as much as possible.

We can call the method build_commit and the param CRYSTAL_CONFIG_BUILD_COMMIT in order to align with the naming inside define_crystal_constants.

RX14 · 2018-10-22T23:34:07Z

I don't mind the naming.

straight-shoota · 2018-10-23T17:17:23Z

src/compiler/crystal/config.cr

@@ -5,15 +5,18 @@ module Crystal
    end

    def self.version
-      version_and_sha.first
+      {% if flag?(:windows) %}
+      {{ `type #{__DIR__}/../../../VERSION`.stringify.chomp }}


Indent this line?
Ditto below.

Compiler version is now fixed in the source git describe is used to show sha if available

Move git rev-parse to Makefile

straight-shoota

Looks great 👍

Sija reviewed Oct 20, 2018

View reviewed changes

straight-shoota reviewed Oct 20, 2018

View reviewed changes

straight-shoota reviewed Oct 22, 2018

View reviewed changes

RX14 approved these changes Oct 22, 2018

View reviewed changes

bcardiff force-pushed the version_file branch from fdc5fab to b20dc6b Compare October 23, 2018 16:57

straight-shoota reviewed Oct 23, 2018

View reviewed changes

bcardiff force-pushed the version_file branch from b20dc6b to adb9614 Compare October 23, 2018 17:54

bcardiff added 7 commits October 23, 2018 15:10

Add VERSION file

ad49039

Compiler version is now fixed in the source git describe is used to show sha if available

Check version is valid

3a38863

Check VERSION content and git tag matches

0ae1351

Use git rev-parse instead of git describe

9dcfb48

Split version_and_sha method

de1f958

Inject build commit information via env

330f0a0

Move git rev-parse to Makefile

Rename Config.sha to Config.build_commit

c449597

bcardiff force-pushed the version_file branch from adb9614 to c449597 Compare October 23, 2018 18:28

bcardiff added the topic:infrastructure label Oct 23, 2018

straight-shoota approved these changes Oct 23, 2018

View reviewed changes

bcardiff merged commit 7b8326f into crystal-lang:master Oct 24, 2018

bcardiff added this to the 0.27.0 milestone Oct 24, 2018

bcardiff deleted the version_file branch October 24, 2018 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add VERSION file #6966

Add VERSION file #6966

bcardiff commented Oct 20, 2018 •

edited

Loading

Sija Oct 20, 2018 •

edited

Loading

bcardiff Oct 20, 2018

asterite commented Oct 20, 2018

bcardiff commented Oct 20, 2018

straight-shoota commented Oct 20, 2018 •

edited

Loading

straight-shoota Oct 20, 2018

RX14 commented Oct 20, 2018 •

edited

Loading

oprypin commented Oct 20, 2018

RX14 commented Oct 21, 2018

straight-shoota commented Oct 21, 2018

straight-shoota Oct 22, 2018

bcardiff commented Oct 22, 2018

straight-shoota commented Oct 22, 2018

bcardiff commented Oct 22, 2018

j8r commented Oct 22, 2018

bcardiff commented Oct 22, 2018

j8r commented Oct 22, 2018

bcardiff commented Oct 22, 2018

RX14 commented Oct 22, 2018

straight-shoota Oct 23, 2018

straight-shoota left a comment

Add VERSION file #6966

Add VERSION file #6966

Conversation

bcardiff commented Oct 20, 2018 • edited Loading

Sija Oct 20, 2018 • edited Loading

Choose a reason for hiding this comment

bcardiff Oct 20, 2018

Choose a reason for hiding this comment

asterite commented Oct 20, 2018

bcardiff commented Oct 20, 2018

straight-shoota commented Oct 20, 2018 • edited Loading

straight-shoota Oct 20, 2018

Choose a reason for hiding this comment

RX14 commented Oct 20, 2018 • edited Loading

oprypin commented Oct 20, 2018

RX14 commented Oct 21, 2018

straight-shoota commented Oct 21, 2018

straight-shoota Oct 22, 2018

Choose a reason for hiding this comment

bcardiff commented Oct 22, 2018

straight-shoota commented Oct 22, 2018

bcardiff commented Oct 22, 2018

j8r commented Oct 22, 2018

bcardiff commented Oct 22, 2018

j8r commented Oct 22, 2018

bcardiff commented Oct 22, 2018

RX14 commented Oct 22, 2018

straight-shoota Oct 23, 2018

Choose a reason for hiding this comment

straight-shoota left a comment

Choose a reason for hiding this comment

bcardiff commented Oct 20, 2018 •

edited

Loading

Sija Oct 20, 2018 •

edited

Loading

straight-shoota commented Oct 20, 2018 •

edited

Loading

RX14 commented Oct 20, 2018 •

edited

Loading