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

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

Closed
Flamefire opened this issue May 31, 2019 · 18 comments
Closed

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

Flamefire opened this issue May 31, 2019 · 18 comments

Comments

@Flamefire
Copy link
Contributor

Flamefire commented May 31, 2019

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
Copy link
Member

vsoch commented May 31, 2019

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

@vsoch
Copy link
Member

vsoch commented May 31, 2019

See #120 (comment)

@vsoch
Copy link
Member

vsoch commented May 31, 2019

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
Copy link
Member

vsoch commented May 31, 2019

This weirdly works (but is wrong)

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

@vsoch
Copy link
Member

vsoch commented May 31, 2019

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
Copy link
Member

vsoch commented May 31, 2019

@vsoch
Copy link
Member

vsoch commented May 31, 2019

@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
Copy link
Contributor Author

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:

  • "prelealease" is a string: VersionInfo(3,2,1,1) -> VersionInfo(3,2,1,"1"). I'll suggest to semver to parse that accordingly
  • VersionInfo(3,2,1-1) is VersionInfo(3,2,0) so nothing weird there ;)

@Flamefire
Copy link
Contributor Author

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
Copy link
Member

vsoch commented May 31, 2019

Yes, I was just testing that! And it worked! https://circleci.com/gh/singularityhub/singularity-cli/651 and then PR here: #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
Copy link
Contributor Author

Due to the tests we need:

  • pre 3.2.0
  • 3.2.0
  • latest

@vsoch
Copy link
Member

vsoch commented May 31, 2019

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
Copy link
Member

vsoch commented May 31, 2019

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
Copy link
Member

vsoch commented May 31, 2019

@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
Copy link
Contributor Author

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

@vsoch
Copy link
Member

vsoch commented May 31, 2019

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
Copy link
Contributor Author

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
Copy link
Member

vsoch commented Jun 6, 2019

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

@vsoch vsoch closed this as completed Jun 6, 2019
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

No branches or pull requests

2 participants