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

Configure Crystal compiler version in Makefile #6748

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Sep 19, 2018

Some complicated logic was used to detect the current compiler version
from git tags. This method is unreliable and could produce invalid
results.

This is completely removed from Crystal source code and the version is
set as env var CRYSTAL_CONFIG_VERSION in the Makefile. The value is
determined from the most recent release listed in CHANGELOG.md. This
is a reliable method even if the code is not checked out in a git
repository.

By default, -dev suffix is appended to the version. For creating a
release build with a non-dev version, CRYSTAL_CONFIG_VERSION needs
to be explicitly specified.

Example usage (assuming inside a git repo):

$ # Development build
$ make crystal
$ bin/crystal version
Crystal 0.26.1-dev [ga107c84] (2018-09-19)
$ # Release build
$ make crystal CRYSTAL_CONFIG_VERSION=0.27.0-rc1
$ bin/crystal version
Crystal 0.27.0-rc1 [ga107c84] (2018-09-19)

Previously, the development version would have been Crystal 0.26.1+64 [ga107c84], or Crystal 0.26.1+? if not built inside a git repository. The release version would have been Crystal 0.27.0 [ga107c84].

The number of commits since release has been lost. But I don't think it has much meaning and is unreliable. It wouldn't be difficult to add it back, though. A maybe even more important option could be including the branch name in a development build, if inside a git repository.

An alternative or supplementary source for the most recent release could also be a VERSION file. But CHANGELOG.md should be pretty solid.

Fixes #6715

Some complicated logic was used to detect the current compiler version
from git tags. This method is unreliable and could produce invalid
results.
This is completely removed from Crystal source code and the version is
set as env var `CRYSTAL_CONFIG_VERSION` in the `Makefile`. The value is
determined from the most recent release listed in `CHANGELOG.md`. This
is a reliable method even if the code is not checked out in a git
repository.
By default, `-dev` suffix is appended to the version. For creating a
release build with a non-dev version, `CRYSTAL_CONFIG_VERSION` needs
to be explicitly specified.
@straight-shoota straight-shoota force-pushed the jm/feature/compiler-version branch from a107c84 to b74af2d Compare September 19, 2018 21:13
@straight-shoota
Copy link
Member Author

Regarding the CI failures: How does src/compiler/crystal/config.cr get included when building std_spec?

@bcardiff
Copy link
Member

I am not in favor of using the changelog as a source of information for the compiler or the build process.

The current source code already allows a specific version to be used as branding (set a value in CRYSTAL_CONFIG_VERSION env var and that will get embedded in the generated binary).

In other projects I worked in, having the literal version that is been built is nice as simple. Then we enforce that tagged build match that description and non tag build have a -dev suffix. This is equivalent to what you are proposing in.

One issue I have with changing the tagging is that after a release, until some period of time we don't know if the next release will be a minor or patch. So there is some inconsistency if the version is not updated immediately after release.

I understand the recent glitch in travis, and that the process can be changed/simplified, but this also affect the whole build process.

I don't think we should invest effort in this area for the time being because:

  • The official releases are always tagged correctly right now (fingers crossed :-P)
  • The CI is happy
  • There is working way to allow non official built to inject the version information.

Maybe we should document better how to create releases of other distros to help mainteners a bit more. I believe the core nature of the PR is that.

@straight-shoota
Copy link
Member Author

I am not in favor of using the changelog as a source of information for the compiler or the build process.

Why not? It's always available when the compiler source code is and it reliably points to the most recent release of the source code. The currently used algorithm uses much weaker sources. Git is not always available and it doesn't reliably point to the most recent tag and the current compiler version doesn't necessarily have any connection to the source version.
As mentioned above, a separate VERSION file could be an alternative to using the changelog.

The current source code already allows a specific version to be used as branding (set a value in CRYSTAL_CONFIG_VERSION env var and that will get embedded in the generated binary).

And this doesn't change with this PR. In fact, the build processes should not require any modification at all (unless I'm missing something?).
But it is adds the possibility to als specify CRYSTAL_CONFIG_COMMIT if not building in a git repo.

One issue I have with changing the tagging is that after a release, until some period of time we don't know if the next release will be a minor or patch. So there is some inconsistency if the version is not updated immediately after release.

I don't really understand hat concern. The version should only be updated at a release.
It might be a little bit strange that 0.26.1-dev actually describes a version after 0.26.1 was released. But that's no different from the current solution, yet the proposed implementation is much more reliable and doesn't produce invalid versions.

This PR shouldn't break existing release build processes.
It improves and simultaneously simplifies the version detection for non-release builds which don't receive an explicit version.
The most important thing is fixing a bug that when git doesn't work out, the version would be 0.26.1-? and that's not compatible with the semantic version implementation which makes a compiler with such a version useless for compiling the stdlib.

@j8r
Copy link
Contributor

j8r commented Oct 20, 2018

Ideally the solution should be in a separate project – crystal-lang/crystal isn't the only project facing this issue, and others can use the adopted solution. In a way this will standardize it. I'm thinking to crystal-lang/shards, or versioning bots.

@straight-shoota straight-shoota deleted the jm/feature/compiler-version branch October 24, 2018 08:42
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.

Compiler version detection
3 participants