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

introduce EasyBlock.post_init method to correctly define builddir variable when build-in-installdir mode is enabled in easyconfig or easyblock #3900

Merged

Conversation

boegel
Copy link
Member

@boegel boegel commented Nov 19, 2021

fixes #3895

@easybuilders easybuilders deleted a comment from boegelbot Nov 20, 2021
branfosj
branfosj previously approved these changes Nov 20, 2021
@branfosj branfosj dismissed their stale review November 21, 2021 12:21

problem to investigate

@branfosj
Copy link
Member

This looks correct, but is not solving the issue I am seeing with OpenFOAM.

eb OpenFOAM-8-foss-2020b.eb -Tr --sanity-check-only

with this PR is not hitting the right section of code.

@branfosj
Copy link
Member

So, setting buildininstalldir = True in the OpenFOAM easyconfig means this works correctly. However, the self.build_in_installdir = True in the OpenFOAM easyblock acts after this.

@smoors
Copy link
Contributor

smoors commented Nov 29, 2021

a possible solution would then be to replace self.build_in_installdir = True with an EasyBlock method self.build_in_installdir():

build_in_installdir(self):
    if self.builddir != self.installdir:
        self.log.info("Changing build dir to %s", self.installdir)
        self.builddir = self.installdir

@boegel boegel changed the title fix builddir value when --sanity-check-only is used and buildininstalldir is enabled fix builddir value when --sanity-check-only is used and build-in-installdir is enabled in easyconfig or easyblock Dec 4, 2021
…ir that is set by easyblock is correctly taken into account
@boegel boegel changed the title fix builddir value when --sanity-check-only is used and build-in-installdir is enabled in easyconfig or easyblock introduce EasyBlock.post_init method to correctly define builddir variable when build-in-installdir mode is enabled in easyconfig or easyblock Dec 4, 2021
… is correctly set based on build_in_installdir (which may be defined dynamically by easyblock in constructor)
@boegel boegel force-pushed the fix_builddir_sanity_check_only branch from c053fb0 to 9623629 Compare December 4, 2021 13:36
@boegel
Copy link
Member Author

boegel commented Dec 4, 2021

a possible solution would then be to replace self.build_in_installdir = True with an EasyBlock method self.build_in_installdir()

@smoors That's possible, but the downside of this approach is that easyblocks need to follow suite in order to pick up this bugfix...

I think a better option is to introduce a post_init method which is always run after initializing the EasyBlock instance (like I did in 9623629), before any steps are run; not making that part of a step makes sure it's always run.

That way, existing easyblocks that set build_in_installdir to True in the constructor don't need to be changed.
There are only two exceptions in the central easybuild-easyblocks repository: the custom easyblocks for PETSc and SLEPc (which are among the oldest easyblocks we have).
I'll open a trivial PR to also make those two set build_in_installdir in the easyblock constructor.

@easybuilders easybuilders deleted a comment from boegelbot Dec 4, 2021
@easybuilders easybuilders deleted a comment from boegelbot Dec 4, 2021
@easybuilders easybuilders deleted a comment from boegelbot Dec 7, 2021
Copy link
Member

@branfosj branfosj left a comment

Choose a reason for hiding this comment

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

lgtm

@branfosj
Copy link
Member

branfosj commented Dec 7, 2021

Going in, thanks @boegel!

@branfosj branfosj merged commit 77f63e0 into easybuilders:develop Dec 7, 2021
@boegel boegel deleted the fix_builddir_sanity_check_only branch December 7, 2021 20:41
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.

builddir not set correctly when build_in_installdir = True and running --sanity-check-only
3 participants