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

cvmfs: use PKG_VERSION_xxx to test pkg_compare_version() return values #90

Merged
merged 1 commit into from
Nov 22, 2016

Conversation

jouvin
Copy link
Contributor

@jouvin jouvin commented Nov 21, 2016

Requires quattor/template-library-core#131 first... But would be good to merge it at the same time.

@jouvin jouvin added this to the 16.10 milestone Nov 21, 2016
@jrha
Copy link
Member

jrha commented Nov 22, 2016

Have you tested that the results are identical?

if ((pkg_compare_version(CVMFS_CLIENT_VERSION, '2.1.20') >= 0) && (OS_VERSION_PARAMS['major'] != 'sl5')) {
SELF[escape('cvmfs-config-default')] = dict();
} else {
if ((pkg_compare_version(CVMFS_CLIENT_VERSION, '2.1.20') == PKG_VERSION_LESS) || (OS_VERSION_PARAMS['major'] == 'sl5')) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not equivalent to the previous code, is it what was intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is (it should)! I inverted the test due to the problem described in quattor/template-library-core#132. Before the test was pkg_compare_version <= 0 which meant (if I am right) greater or equal. I swaped the if conditions and thus test for less.

@jouvin
Copy link
Contributor Author

jouvin commented Nov 22, 2016

Yes it has been tested at GRIF and I saw no difference (as expected).

@jrha jrha merged commit beb6af4 into quattor:master Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants