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

Merge of the ConfigureMake and CMakeMake versions of the easyblock for QuantumESPRESSO #3338

Merged
merged 12 commits into from
Jul 17, 2024

Conversation

Crivella
Copy link
Contributor

The QuantumESPRESSO ConfigMake and CMakeMake easyblocks have been merged in the same file with a wrapper easyblock deciding which one will get used depending on the version of QE.

This PR would

@ocaisa ocaisa changed the title Merge of the ConfigMake and CMakeCMake easyblocks Merge of the ConfigMake and CMakeCMake easyblocks for QuantumESPRESSO May 24, 2024
@ocaisa ocaisa changed the title Merge of the ConfigMake and CMakeCMake easyblocks for QuantumESPRESSO Merge of the ConfigureMake and CMakeMake versions of the easyblock for QuantumESPRESSO May 24, 2024
@Crivella Crivella force-pushed the feature-QE_merge_easyblocks branch from 5617ae1 to 456e5d4 Compare May 24, 2024 14:32
@Crivella
Copy link
Contributor Author

@boegel I tried merging the 2 easyblocks for QuantumESPRESSO into a new one acting as a wrapper and forwarding the calls to the correct EB based on the version. It is slightly hackish but still very clean code-wise.

Unfortunately eb --list-easyblocks and it's associated unittest is causing the CI to fail, due to having multiple classes defined in one file.
I am not sure if the change i proposed in easybuilders/easybuild-framework#4543 would make sense.

The alternatives would be to either define the classes inside something like another class (or the wrapper class itself) or a dict (anything that will let class not be at the beginning of the string to trick the ^class regex), but that would make the code less maintainable in my opinion.
There is also the option of squashing the classes together and have a wrapper for every ..._step method, but that would make it even less maintainable.

Any suggestion in which direction to proceed?

@boegel boegel added the change label Jun 5, 2024
@boegel boegel added this to the 4.x milestone Jun 5, 2024
@ocaisa
Copy link
Member

ocaisa commented Jun 14, 2024

@boegel boegel modified the milestones: 4.x, release after 4.9.2 Jun 19, 2024
@Crivella Crivella force-pushed the feature-QE_merge_easyblocks branch 2 times, most recently from 505eca8 to f9df434 Compare July 16, 2024 13:11
@Crivella Crivella force-pushed the feature-QE_merge_easyblocks branch from f9df434 to 5f95e61 Compare July 16, 2024 13:13
@Crivella
Copy link
Contributor Author

Put version specific classes inside the wrapper as to not have trouble with framework EB detection regex

Copy link
Member

@ocaisa ocaisa left a comment

Choose a reason for hiding this comment

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

@ocaisa ocaisa merged commit 32e45bd into easybuilders:develop Jul 17, 2024
41 checks passed
@Crivella Crivella deleted the feature-QE_merge_easyblocks branch July 17, 2024 10:14
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.

3 participants