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

Get version number from CHANGELOG.md when cannot get it from git #4687

Conversation

makenowjust
Copy link
Contributor

Fixed #4642

@asterite
Copy link
Member

asterite commented Jul 7, 2017

It was working well. I always tagged the commit that I was using for doing a release. Omnibus checks out the repo from git so the tag would be there. I'm not sure we need to change anything.

@asterite
Copy link
Member

asterite commented Jul 7, 2017

(it would be interesting to know why it started to give incorrect versions)

@matiasgarciaisaia
Copy link
Member

@asterite this tries to cover the usecase of downloading a zip/tar from Github, without repo history.

I think I'd just tag the version as dev in that case, and that's all. I wouldn't say a zip file is a given release version.

Maybe mixing this with a -dev suffix could do (something like 0.23.0-dev).

But I'm not sure what I'd like the version to report on this builds - let alone the heuristics we apply.

@makenowjust makenowjust force-pushed the fix/crystal/4642-version-build-tar-ball branch from 08df94f to fe35339 Compare July 7, 2017 19:46
@straight-shoota
Copy link
Member

A zip file or any other method without git is probably not an official way of distribution, but it's still the source code of the compiler at a specific version. I think it should not depend on git being available on the build system.

Additionally, I don't think it is wise to rely on Crystal::VERSION at all because it cannot safely be assumed that the compiler is compiler with the latest released compiler. And even if it is, it would still report a version prior if you're building the latest release (without git) - e.g. 0.23.0 - with the second latest compiler - e.g. 0.22.0.
If there is no git tag and no version to be found in the changelog the compiler should fail and ask to specify CRYSTAL_CONFIG_VERSION instead of assuming anything.
This should also apply to the branch for shallow clone with no tag in reach.

The -dev suffix for versions from changelog would be great because without git you can't know if its exactly the latest release or has modifications.

@makenowjust
Copy link
Contributor Author

@matiasgarciaisaia dev sounds bad because the user (i.e. @need47) builds crystal from released tar.gz, so crystal version should report correct version as possible.

However, this compiler is unofficial. We may tell about this is built outside of git repo. When crystal is built in git repo, crystal version reports git's SHA of built commit: Crystal 0.22.0+156 [cb0ee67cb]. So, when built outside of git repo, crystal version reports [out of git] additional information: Crystal 0.23.1 [out of git].

@ysbaddaden
Copy link
Contributor

I regularly compile from tarballs (e.g. alpine, raspbian, ...) not from git clones, so this change is welcomed!

@straight-shoota
Copy link
Member

@makenowjust There is no way to tell if the code being build is from the released tar.gz or has a ton of other modifications, so I think it would make sense to assume it is not the release version by default.
If you know that you're building the release version (like @ysbaddaden) you just need to set CRYSTAL_CONFIG_VERSION appropriately as a way of saying "I know what I'm doing". Don't think this would be to much effort and provides a safe default.

@RX14
Copy link
Contributor

RX14 commented Jul 8, 2017

If we don't know the compiler version and have no way of telling we should fall back to "Unknown" not the version of the compiler it was compiled with. If you're compiling with a tarball you can always set CRYSTAL_CONFIG_VERSION. Parsing the changelog seems brittle to me.

@makenowjust
Copy link
Contributor Author

unknown sounds good to me. I think it is better than dev at least.

Additionally, in such a case we should display a warning message: failed to get the version number from git, Crystal::VERSION would be set to "unknown" as fallback. for example.

@makenowjust
Copy link
Contributor Author

@ysbaddaden In fact I don't build crystal from tar balls, so I don't know how it works. How do you think about it in real world?

@ysbaddaden
Copy link
Contributor

That depends.

Alpine's AKPBUILD automatically downloads the tarball for the version to compile. Since it's automated, I properly set the environment variable (but had to thought to set it).

On Ubuntu I now just download a tarball and build. I don't have to think to switch branches or checkout a tag, and have a clean install, distinct from the git clone. Easier. But version number is wrong, because I always forget to set the environment variable beforehand.

It's a tiny micro minor hitch. We shouldn't bother much, but having a wrong reported version number when building from an official tarball may lead to some confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants