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

Update IPP easyblock for versions > 2021 #2909

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

agemuend
Copy link

(created using eb --new-pr)

@boegelbot

This comment was marked as outdated.

@jfgrimm jfgrimm added this to the 4.x milestone Mar 21, 2023
@jfgrimm jfgrimm added the update label Mar 21, 2023
@boegel boegel modified the milestones: 4.x, next release (4.7.2?) Mar 21, 2023
@boegel boegel changed the title Update IPP easyblock for versions > 2021. Update IPP easyblock for versions > 2021 Mar 28, 2023
@boegel boegel modified the milestones: 4.7.3, release after 4.7.3 Jul 5, 2023
@akesandgren
Copy link
Contributor

Test report by @akesandgren

Overview of tested easyconfigs (in order)

Build succeeded for 0 out of 1 (1 easyconfigs in total)
b-an02.hpc2n.umu.se - Linux Ubuntu 20.04, x86_64, Intel(R) Xeon(R) CPU E5-2690 v4 @ 2.60GHz, Python 3.8.10
See https://gist.github.com/akesandgren/2d271eaf7876e9d7150449b730f8701a for a full test report.

@akesandgren
Copy link
Contributor

As you can see, this breaks installation of older ipp versions.

@agemuend
Copy link
Author

As you can see, this breaks installation of older ipp versions.

Mhm, thats interesting. The error is "directory already exists", but my patch actually introduces the suppression of making the installdir, because I thought it was newer installers that had problems with this. Does the 2017 version actually work before the patch?

@akesandgren
Copy link
Contributor

Yes it works before this PR. See my fix for the problem above.

@agemuend
Copy link
Author

Yes it works before this PR. See my fix for the problem above.

Sorry, where exactly?

Comment on lines +56 to +59
def make_installdir(self):
"""Do not create installation directory, install script handles that already."""
if LooseVersion(self.version) >= LooseVersion('2021'):
super(EB_ipp, self).make_installdir(dontcreate=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong, you need an else-stmt which calls

super(EB_ipp, self).make_installdir()

Or add a variable "dontcreate=None", changing it to True if version > 2021 and then call supr(...).make_installdir(dontcreate=dontcreate)

The latter depends a bit too much of a-prio knowledge of what the default for dontcreate is in the super classes.
So the first way is less prone to get hit by a change in intelbase.py or frameworks easyblock.py

Copy link
Contributor

Choose a reason for hiding this comment

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

With that change installing ipp-2017.1.132.eb works again.

@akesandgren
Copy link
Contributor

Sorry, I managed to forget to click send the review.

@akesandgren
Copy link
Contributor

@agemuend ping

@boegel boegel modified the milestones: 4.9.1, release after 4.9.1 Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants