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

enable out of tree build by default in CMakeMake easyblock #1933

Merged
merged 3 commits into from
Apr 1, 2020

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Jan 16, 2020

This supersedes #616 and includes #1929 which in turn includes an improved version of #616 concerning defaults for out-of-tree builds while this contains the setting only.

Better: It is intended that 1929 is merged first, and this rebased on the merged result (currently it is rebased on 1929) to see the small changes made by this PR. 1929 contains explicit setting of out-of-tree builds where required which is need for this PR to work.

I already did extensive tests with existing EasyConfigs using True as the default. The ECs using CMake are
cmakeECs.txt as found by grepping the ECs and EBs. I chose a subset of those to test excluding ones using old Intel toolchains as I don't have old intel compilers. I previously excluded all intel ECs hence the names below

So:

From this extensive test the failure introduced was always a build failure so can be easily detected. To the best of my knowledge (according to the tests) there are no failures introduced for existing ECs. Most non-EB ECs should be caught by the changed EasyBlocks that set the out-of-tree build to False where required.

From those results I see no reason NOT to enable it by default. I'm against using the deprecating behavior as this would force all ECs/EBs to include setting the new default which will be rendered superflous, I Included it nonetheless to spark some discussion but would like to remove it.

easybuild/easyblocks/generic/cmakemake.py Outdated Show resolved Hide resolved
@boegel
Copy link
Member

boegel commented Mar 31, 2020

@Flamefire Please sync this with develop now #1929 is merged, to get a good view on the remaining changes.

Should be good to go, already tested extensively...

@Flamefire
Copy link
Contributor Author

Done. Please don't forget to merge easybuilders/easybuild-easyconfigs#9738

@boegel boegel changed the title Default out of tree build for CMakeMake enable out of tree build by default in CMakeMake easyblock Apr 1, 2020
easybuild/easyblocks/generic/cmakemake.py Outdated Show resolved Hide resolved
easybuild/easyblocks/generic/cmakemake.py Show resolved Hide resolved
@boegel
Copy link
Member

boegel commented Apr 1, 2020

This change also required easybuilders/easybuild-easyconfigs#10300 (which is fine, since that easyconfig is only there in develop)

@boegel boegel dismissed akesandgren’s stale review April 1, 2020 17:36

remarks taking into account

@boegel
Copy link
Member

boegel commented Apr 1, 2020

Extensively tested and approved, so going in, thanks @Flamefire!

@boegel boegel merged commit 5701c03 into easybuilders:develop Apr 1, 2020
@Flamefire Flamefire deleted the outoftree branch April 1, 2020 18:17
@klust
Copy link

klust commented Apr 16, 2020

This change, combined with the fact that the out-of-source build tree has a fixed name, wreaks havoc with Bundle-based eb-files that contain more than one package built with CMake as the second build fails because EasyBuild tries to build it in the directory of the first one. It is of course easily circumvented by setting separate_build_dir back to its old default False in those EB-files. But if out-of-tree builds are considered important, it would be better to also encode the name/version of the package in the build directory, and even better to also encode the iteration number for builds with multiple iterations (e.g., an array of configopts) in the directory name for the build.

@Flamefire
Copy link
Contributor Author

Good point. Possible mitigations:

  • Build inside a subfolder of the source. Usual workflow I see outside EB is: mkdir build && cd build; cmake .. <other opts>; make && make install
  • Remove build folder after install step / before build step at the point where it is to be created. IMO this is a good approach as the point of an out-of-source dir is a clean build so that folder should not exist or be empty

@boegel
Copy link
Member

boegel commented Apr 18, 2020

Ideally, an issue should be opened for the (valid) comments raised by @klust, keeping track of things to change in merged PRs is very difficult w.r.t. staying on top of things.

@klust Do you mind opening an issue for this via https://github.com/easybuilders/easybuild-easyblocks/issues/new?

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.

5 participants