-
Notifications
You must be signed in to change notification settings - Fork 107
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
add support for rhel9. Fix #11895 #11897
add support for rhel9. Fix #11895 #11897
Conversation
Jenkins results:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@belforte Stefano, thank you for providing this.
In addition to the comment along the code, please also fix the new unit test failing (as can be seen in the jenkins report):
Scram_t.Scram_t:testArchMap changed from success to failure
@@ -43,7 +43,9 @@ | |||
ARCH_TO_OS = {'slc5': ['rhel6'], | |||
'slc6': ['rhel6'], | |||
'slc7': ['rhel7'], | |||
'el8': ['rhel8'], 'cc8': ['rhel8'], 'cs8': ['rhel8'], 'alma8': ['rhel8']} | |||
'el8': ['rhel8'], 'cc8': ['rhel8'], 'cs8': ['rhel8'], 'alma8': ['rhel8'], | |||
'el9': ['rhel9'], 'cc9': ['rhel9'], 'cs9': ['rhel9'], 'alma9': ['rhel9'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stefano, right now I only see this ScramArch under the version 9 of this OS:
el9_amd64_gcc12
In other words, I see no flavors for cc9_, cs9_, alma9_*. Said that, do we really plan on supporting those in CMSSW? If not, then we shouldn't even bother in supporting them in WMCore/CRAB as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will look at the unit test, thanks.
For this question, I admit that I copied the el8
line w/o knowing if those ARCH's may exist. I am not sure those exist either. I am not sure where you are looking. E.g. I find el9
and cs9
here
belforte@lxplus807/~> ls /cvmfs/cms.cern.ch/|grep 9_amd cs9_amd64_gcc11/ el9_amd64_gcc11/ el9_amd64_gcc12/ el9_amd64_gcc13/ belforte@lxplus807/~>
while for 8
same command reports cc8
cs8
el8
rhel8
alma8
and ubi8
.
The latter is not mapped atm. How do we find the correct list ?
I am happy to start with el9
and cs9
and add more as/if needed. Indeed there's no point in listing SCRAM_ARCH's which do not exist in SCRAM !
I do not even know if el9
and cs9
are different things, or all aliases for alma9
at the end of the day, since we can only pick a running container as rhel7|8|9
Jenkins results:
|
I do not know what to make of the failing "default" test above. Seems all those slices are failing since 1 year. But I did not dig much. |
@belforte Stefano, these changes are looking good to me. One last thing, can you please squash the 1st and 2nd commit together (leaving the unit test one in its own commit). If you need some instructions on that, this is a link that I used many times: https://steveklabnik.com/writing/how-to-squash-commits-in-a-github-pull-request |
665144c
to
5f04a98
Compare
I squashed and fixed pylint (hope) |
You told me once, and I keep it under my pillow :-) |
Jenkins results:
|
The pylint is a leftover from previous implementations, don't worry with that. Given that you have already made that change though, you might want to squash it with the first commit (it's a single line). |
5f04a98
to
7a51ee9
Compare
Squashed. the pylint complain about Scram_t.py is "debatable" surely this looks redundant to a dummy compiler except Exception:
raise but you need a way to make sure the I am keeping it as it is |
Jenkins results:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unstable unit test is unrelated. Thank you very much for providing these changes, Stefano!
thanks Alan for your guidance |
@belforte Stefano, we just cut a new tag and you can find this development in |
Fixes #11895
Status
Ready
Description
Add matches for rhel9 in ARCH_TO_OS
Is it backward compatible (if not, which system it affects?)
YES
Related PRs
None
External dependencies / deployment changes
None