Simplify Cylc code to retrieve the version, drop VERSION file and git

[kinow]

Hi!

As per the title, this issue is for the discussion of changing how Cylc version is currently stored and retrieved.

The problem

From what I understand, we have three places to look for the version:

• cylc shell script with cylc version
• cylc.version.CYLC_VERSION constant defined during the script load/import
• VERSION file created by make and by a utility in etc/dev-bin/create-version-file

The behaviour at the moment is not consistent.

• cylc version uses a function get_version from the cylc shell script. That function looks first at VERSION file if existent, otherwise does a git describe --abbrev=4 --always (plus -dirty suffix). If no version found, prints No version is set, cylc must be built first. Aborting...
• cylc.version.CYLC_VERSION first looks at git using etc/dev-bin/get-repo-version script which uses git describe --always --abbrev=4 --tags HEAD (plus -dirty suffix). If no version found, then it looks for the VERSION file. Finally, if still no version found, it returns "UNKNOWN".
• The VERSION file is created by make, using a utility which won't create the file if the directory contains the folder .git. Otherwise gets the version from the parent directory name.

Furthermore, Cylc's profile-battery command uses cylc version command twice, once to fetch the location of Cylc, and later to get its major version. There are a few I/O operations that could be saved, plus some code in the cylc.profiling package to be reviewed.

A possible solution

I have some working code, mixed with a possible fix for the cylc.profiling package (see #2991). My plan is to get this code, isolate what's related to the version part of Cylc, and then prepare a pull request.

That would require to:

• Drop the VERSION file
• Drop the git version (helpful to Make cylc a module #2990 too, as the current x.y.z-12345 is not parsed by setuptools without some extra hacking)
• Modify lib/cylc/__init__.py, creating __version__ = '8.0.0-a0 (as in the current git tag, or maybe 8.0.a0 [would this one be equivalent to 8.0.0 alpha @matthewrmshin ?])
• Remove all code using the VERSION file
• Remove all code using the git version, the cylc.version module, replacing everything by something like import cylc.__version__ as CYLC_VERSION or so

Risks:

• Users relying on the old VERSION file would have a different behaviour. Though that will change anyway once they use PYPI, Conda, RPM, virtualenv, etc.
• Users won't see the git tag/commit when installing Cylc via PYPI, Anaconda, etc... but we can still add it back to the Web GUI, or even in the command line (just import the cylc.__version__ and append the git commit?)
• Developers will have to modify their workflow for updating versions, as the point of truth would be in the file lib/cylc/__init__.py. I think that's common among Python projects, and already in use in isodatetime

Thoughts?

Cheers
Bruno

[hjoliver]

Cylc Version Requirements

(in case this helps inform what we need)

• CLI cylc version is for users to know what Cylc version is in their current environment
• Cylc library lib/cylc/version.py is used by the scheduler program to know its own version, so that it can tell jobs to invoke client commands (e.g. cylc message) with the same version
• cylc profile-battery checks out and compares performance of selected versions.

Currently

• unpacked releases (after make) have a VERSION file that identifies the release version. This comes from the source directory name (inside the tarball) and originates from git describe at release time (but should be a clean x.y.z value because releases are tagged just prior to release).
• git clones identify their "version" using git describe (which generates a string based on most recent tag, number of commits since then, and latest commit ID).

[hjoliver]

@kinow - does your proposal require manually updating the version in lib/cylc/__init__.py?

One question that comes to mind then is, $CYLC_VERSION will no longer be set correctly in job environments (see library use above) when running out of a git clone. However we do not actually try to automatically check out a tagged version for use by clients on job hosts, so this probably doesn't matter! (although I think that was the original intention, way back when...).

[kinow]

Thanks @hjoliver ! I finished the changes necessary to fix the cylc.profiling, but I think they are conflicting now due to the bandit changes merged just now. Will update it later.
But here's a possible fix: https://github.com/cylc/cylc/compare/master...kinow:simplify-cylc-version?expand=1

does your proposal require manually updating the version in lib/cylc/init.py?

Yes, as in isodatetime and a few other projects do (which is interesting, PHP with composer also does that, but Java does the update automagically when you use Maven).

One question that comes to mind then is, $CYLC_VERSION will no longer be set correctly in job environments (see library use above) when running out of a git clone. However we do not actually try to automatically check out a tagged version for use by clients on job hosts, so this probably doesn't matter! (although I think that was the original intention, way back when...).

If that's set by cylc job-submit, this behaviour is still kept in my branch. The only thing I did not dare to change were the INSTALL.md, doc/ files, etc... because this change invalidates so much of our documentation 😭 but we will have to update that anyway for the setup.py work.

[hjoliver]

If that's set by cylc job-submit, this behaviour is still kept in my branch.

What I meant above was, if running from a git clone, CYLC_VERSION - in suite jobs - will no longer represent the exact "version" being executed (down to the specific commit) - right?

However, I don't suppose that actually matters (we don't actually check out the exact version on job hosts)... in which case I think your proposal is good. Less automatic, but cleaner and more standard.

[hjoliver]

(BTW - to state the obvious - the original intent of the current system was to automatically determine cylc version in both git clones and releases (and exactly, to commit level, in clones) without having to manually set it anywhere ... but I agree the associated code has become a mess).

[kinow]

Oh, you are right @hjoliver. It won't represent the exact version being executed. Hopefully users will be OK assuming 8.0.0 is 8.0.0 no matter whether they are using the version from GitHub tag, rpm, PYPI, etc.

I had a look at the documentation and looks like the VERSION and CYLC_VERSION are both mentioned in the .rst files, and also in the INSTALL.md. These files were not updated here, because I think the changes to the documentation will be even more extensive after setup.py

[hjoliver]

Risks:

Users relying on the old VERSION file would have a different behaviour. Though that will change anyway once they use PYPI, Conda, RPM, virtualenv, etc.

Users should not be relying on the old VERSION file (I can't think what for, anyway). Most are probably not even aware of it.

Users won't see the git tag/commit when installing Cylc via PYPI, Anaconda, etc... but we can still add it back to the Web GUI, or even in the command line (just import the cylc.version and append the git commit?)

Users should not need to see the git tag/commit because the release version is enough to identify the exact source version (it should correspond to a tag in the repository)

Developers will have to modify their workflow for updating versions, as the point of truth would be in the file lib/cylc/init.py. I think that's common among Python projects, and already in use in isodatetime

Yeah, that's fine with me.

[hjoliver]

These files were not updated here ...

You might as well update them, unless others @cylc/core disagree with your proposed change (but I don't think they will). ... Can always update again after setup.py etc...

[kinow]

Will do that tomorrow @hjoliver . Will be a good exercise reviewing the docs too.

Travis-CI passed all tests for the branch I linked a few comments ago: https://travis-ci.org/kinow/cylc/builds/505560417 👍

Only missing documentation and approval from others @cylc/core then.

[matthewrmshin]

On version string. If I am reading PEP440 correctly, we should do 8.0a1 or 8.0.0a1 (no hyphen between 0 and a). I prefer 8.0a1 personally.

[matthewrmshin]

I agree with the proposed change.

[hjoliver]

we should do 8.0a1 or 8.0.0a1 (no hyphen between 0 and a). I prefer 8.0a1 personally.

OK, I'll change my new "alpha" tag from 8.0.0-a0 to 8.0a0 (a1 will be the first actual alpha release for testing; a0 just marks(ish) where cylc-8 diverges from cylc-7).

I suppose your preference for the short form (rather than 8.0.0a1) is because we're unlikely to ever make a an alpha preview of a bug-fix release? (e.g. 8.2.3a1)

[hjoliver]

Done

[matthewrmshin]

I suppose your preference for the short form (rather than 8.0.0a1) is because we're unlikely to ever make a an alpha preview of a bug-fix release? (e.g. 8.2.3a1)

Yes. User base is not big enough for that!

[matthewrmshin]

Cool!