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

Add XALT EasyBlock #1942

Merged
merged 9 commits into from
May 29, 2020
Merged

Add XALT EasyBlock #1942

merged 9 commits into from
May 29, 2020

Conversation

samcmill
Copy link

Specialized variant of the ConfigureMake EasyBlock for building and installing XALT.

cc @rtmclay

easybuild/easyblocks/x/xalt.py Outdated Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Show resolved Hide resolved
Copy link
Contributor

@victorusu victorusu left a comment

Choose a reason for hiding this comment

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

I think we should support more options from XALT

easybuild/easyblocks/x/xalt.py Outdated Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Show resolved Hide resolved
@samcmill
Copy link
Author

Thanks for the feedback @victorusu.

The approach I took was to keep the EasyBlock as lean as possible and keep the specifics in the XALT EasyConfig. Additional XALT EasyConfigs could be created for other permutations, such as the curl transmission method. (If not for the XALT prefix behavior, I likely would have used the standard ConfigureMake EasyBlock.)

Alternatively, the EasyBlock could be a much more customized version of the ConfigureMake EasyBlock that hard codes more XALT options, as you are suggesting.

From what I can tell, the first approach seems to be more closely aligned with the EasyBuild "spirit".

@boegel, do you have a preference?

@boegel boegel added the new label Jan 31, 2020
@boegel boegel added this to the 4.x milestone Jan 31, 2020
@boegel
Copy link
Member

boegel commented Jan 31, 2020

I'll try to take a look at this soon, perhaps also polling @rtmclay (the XALT developer) for feedback.

Thanks a lot for the contribution @samcmill, hopefully we can get this merged soon in one form or another...

@boegel boegel modified the milestones: 4.x, next release (4.1.2?) Jan 31, 2020
@samcmill
Copy link
Author

samcmill commented Mar 2, 2020

@boegel would still appreciate your feedback on this. Thanks!

@boegel
Copy link
Member

boegel commented Mar 3, 2020

@samcmill Thanks for the bump!
I'll reach out to @rtmclay to get some feedback from him on this...

easybuild/easyblocks/x/xalt.py Outdated Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Outdated Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Outdated Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Show resolved Hide resolved
@boegel boegel modified the milestones: next release (4.1.2?), 4.x Mar 3, 2020
@boegel
Copy link
Member

boegel commented Mar 3, 2020

@samcmill I'm happy to clarify anything if needed, either via this PR or more directly (EasyBuild Slack channel, or a short conf call), please don't hesitate to reach out!

@samcmill
Copy link
Author

samcmill commented Mar 5, 2020

@boegel @victorusu The latest commit should address most of your feedback. Thanks!

The step monkeying with the install path is still there. I'll revisit that once xalt/xalt#26 is merged.

I'm unsure about the alternative linker stuff, so no changes for that yet. @victorusu - can you please clarify?

@samcmill
Copy link
Author

The latest changes incorporate the --with-siteControlledPrefix option added to XALT 2.7.29 by @rtmclay, so the easyblock code that monkeys with the prefix has been removed.

This is ready for another review.

@samcmill
Copy link
Author

@boegel @victorusu can you please review this again?

@samcmill
Copy link
Author

@boegel @victorusu can you please review this again?

@boegel
Copy link
Member

boegel commented May 13, 2020

@samcmill I'm sorry for not getting back to this yet, it's on my TODO list for EasyBuild v4.2.1 (see https://github.com/orgs/easybuilders/projects/5)

Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

@samcmill Sorry for the long delay in getting back to you on this... Do let me know if something isn't clear!

easybuild/easyblocks/x/xalt.py Outdated Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Outdated Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Outdated Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Outdated Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Show resolved Hide resolved
easybuild/easyblocks/x/xalt.py Outdated Show resolved Hide resolved
@boegel
Copy link
Member

boegel commented May 29, 2020

Tested with easybuilders/easybuild-easyconfigs#9792, good to go, thanks a lot for your efforts on this @samcmill!

@boegel boegel merged commit 71e2fb8 into easybuilders:develop May 29, 2020
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.

4 participants