Skip to content

Conversation

@e4t
Copy link
Contributor

@e4t e4t commented Sep 7, 2020

This fixes Issue #445.
Thanks to Matt Ezell from Oak Ridge.

Signed-off-by: Egbert Eich [email protected]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Matt in the issue only inverted the equality, but you also changed the suse version from 1550 to 1500 -- is that on purpose?
(might want to say that in the commit message if so)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was unintentional - I don't have OpenSuSE so I'm not sure exactly what version stopped supporting py2. Changing the 1500 back to 1550 is probably the right thing to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%suse_version 1500 includes Leap/SLE 15 - which still has python2. %suse_version 1550 is Tumbleweed/Factory (currently) where python2 has been dropped from.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so from that I take we don't want python2 in Tumbleweed (1550), so the new logic is correct -- wasn't that just what the problem is about?

Well, non-convoluted logic is better for everyone anyway, thanks!

@degremont
Copy link
Collaborator

@e4t the test failed for unrelated reason. Refreshing the commit would retrigger them. It could be good to update your commit message to follow the way we usually do them. See as an example: 23621a5#diff-8798411bf1e861edaa6a04787c30ade5

Basicly (feel free to adapt or reword):

specfile: fix logic for detecting if Python2 is supported

Python2 was conditionally enabled only if building for 
an old RHEL or old SLES but the version tests was not correct.

Thanks to Matt Ezell from Oak Ridge.

Fixes #445.
Signed-off-by: Egbert Eich [email protected]

It's now less obfuscated.
Python2 was conditionally enabled only if building for an old
RHEL or old SLES but the version tests was not correct.

Thanks to Matt Ezell from  Oak Ridge.

Fixes cea-hpc#445.
Signed-off-by: Egbert Eich <[email protected]>
@e4t
Copy link
Contributor Author

e4t commented Sep 9, 2020

I've updated and pushed it again, now.

@thiell
Copy link
Collaborator

thiell commented Sep 14, 2020

Hi @e4t, thanks for the PR! LGTM.

Could you please push your last change to the e4t/master branch so this PR gets updated?

@e4t
Copy link
Contributor Author

e4t commented Sep 14, 2020

Done. I had accidentally created an additional branch.

@thiell
Copy link
Collaborator

thiell commented Sep 14, 2020

Thanks @e4t!

@thiell thiell merged commit 3ec8632 into cea-hpc:master Sep 14, 2020
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.

5 participants