singularityhub / singularity-cli

streamlined singularity python client (spython) for singularity
https://singularityhub.github.io/singularity-cli/
Mozilla Public License 2.0
57 stars 31 forks source link

Check semver for "singularity version 3.2.1-1" #122

Closed Flamefire closed 5 years ago

Flamefire commented 5 years ago

Make sure the following works for "singularity version 3.2.1-1"

cli.version_info() >= VersionInfo(3, 2, 1)

Just so we don't forget it.

vsoch commented 5 years ago

It doesn't, I manually tested it yesterday. I can tell you for sure it doesn't.

vsoch commented 5 years ago

See https://github.com/singularityhub/singularity-cli/pull/120#issuecomment-497378125

vsoch commented 5 years ago

We would probably have to compare it to VersionInfo(3,2,1,1) but then if someone installs (not from the release, but from the branch) and they get 3.2.1 (without the extra 1, as I have locally) it would again not work. Here is on my local machine:

from semver import VersionInfo

VersionInfo(3,2,1,1)
VersionInfo(major=3, minor=2, patch=1, prerelease=1, build=None)

from spython.main import Client

Client.version_info()
VersionInfo(major=3, minor=2, patch=1, prerelease=None, build=None)

print(Client.version_info())
3.2.1

Client.version_info() <= VersionInfo(3,2,1)
True

It actually errors out when I compare to 3.2.1-1

Client.version_info() <= VersionInfo(3,2,1,1)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-9-1f1090771d8f> in <module>()
----> 1 Client.version_info() <= VersionInfo(3,2,1,1)

~/anaconda3/lib/python3.6/site-packages/semver-2.8.1-py3.6.egg/semver.py in __le__(self, other)
    143         if not isinstance(other, (VersionInfo, dict)):
    144             return NotImplemented
--> 145         return _compare_by_keys(self._asdict(), _to_dict(other)) <= 0
    146 
    147     def __gt__(self, other):

~/anaconda3/lib/python3.6/site-packages/semver-2.8.1-py3.6.egg/semver.py in _compare_by_keys(d1, d2)
    250 
    251     rc1, rc2 = d1.get('prerelease'), d2.get('prerelease')
--> 252     rccmp = _nat_cmp(rc1, rc2)
    253 
    254     if not rccmp:

~/anaconda3/lib/python3.6/site-packages/semver-2.8.1-py3.6.egg/semver.py in _nat_cmp(a, b)
    234 
    235     a, b = a or '', b or ''
--> 236     a_parts, b_parts = split_key(a), split_key(b)
    237     for sub_a, sub_b in zip(a_parts, b_parts):
    238         cmp_result = cmp_prerelease_tag(sub_a, sub_b)

~/anaconda3/lib/python3.6/site-packages/semver-2.8.1-py3.6.egg/semver.py in split_key(key)
    221 
    222     def split_key(key):
--> 223         return [convert(c) for c in key.split('.')]
    224 
    225     def cmp_prerelease_tag(a, b):

AttributeError: 'int' object has no attribute 'split'
vsoch commented 5 years ago

This weirdly works (but is wrong)

Client.version_info() <= VersionInfo(3,2,1-1)
False
vsoch commented 5 years ago

We face the same issue for 3.2.0, installed from the release:

from semver import VersionInfo                                          

VersionInfo(3,2,0)                                                      
VersionInfo(major=3, minor=2, patch=0, prerelease=None, build=None)

from spython.main import Client                                         

Client.version_info()                                                   
VersionInfo(major=3, minor=2, patch=0, prerelease='1', build=None)

print(Client.version_info())                                            
3.2.0-1

Client.version_info() > VersionInfo(3,2,0)                              
False

Client.version_info() >= VersionInfo(3,2,0)                             
False
vsoch commented 5 years ago

https://github.com/sylabs/singularity/issues/3653

vsoch commented 5 years ago

@Flamefire I reported the issue above. I'm not sure what we can actually do for this - the official releases for Singularity 3.2.1, 3.2.0 are in fact tagged as pre-releases and wrong.

Flamefire commented 5 years ago

Leave this one to me. IMO '3.2.1-1' >= ' 3.2.1' should hold. I'll have to check the implementation and with semver maintainers.

FTR:

Flamefire commented 5 years ago

Correction: '3.2.1-1' <= ' 3.2.1' holds due to https://semver.org/ §11:

When major, minor, and patch are equal, a pre-release version has lower precedence than a normal version

So we should just compare to VersionInfo(3,2,1,'1') and be done

vsoch commented 5 years ago

Yes, I was just testing that! And it worked! https://circleci.com/gh/singularityhub/singularity-cli/651 and then PR here: https://github.com/singularityhub/singularity-cli/pull/123

Is it worth keeping the section for 3.2.0 now? It will use up free build time, and we can't support every point release.

Flamefire commented 5 years ago

Due to the tests we need:

vsoch commented 5 years ago

Latest is met (3.2.1-1) and pre 3.2.0 would be the current 3.1.0-1, but for 3.2.0 exactly I don't know how to meet that requirement - the official releases are are tagged with pre-release, so currently we do 3.1.0-1, is that not sufficient?

vsoch commented 5 years ago

Or do you mean parsing VersionInfo with a string like "pre-x" ? Could you maybe update the PR I did or just open a new one with what you have in mind? I think that would be faster than trying to explain to me :)

vsoch commented 5 years ago

@Flamefire is there anything I can do to help move forward? I'm not sure what you have in mind for updating the testing versions, and I don't want to make other changes until you've had a look at #119. Is there anything I can work on today / this weekend?

Flamefire commented 5 years ago

Add tests for conversion with files as input/output . Try to break your code ;)

vsoch commented 5 years ago

I can do that! But how do we generate the inputs/ outputs? If we start with a Singularity recipe, the output for the Dockerfile will be organized into sections. If we start with a Dockerfile, the Singularity recipe will organize it, and going back to Dockerfile will be a different Dockerfile.

I'll definitely do a first shot at this. For the version info and further review, will you be able to help more on Monday?

Flamefire commented 5 years ago

Have one folder for each direction of conversion. Start with a simple input, then add corner cases and examples of tricky stuff. Just like regular tests

Yes, a bit. I can't allocate much time for this but I'll see what I can do

vsoch commented 5 years ago

We handled this in the recent updates to testing. I hope Singularity doesn't have this issue in the future.