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

simplify various CMakeMake-based easyblocks by enhancing CMakeMake (w.r.t. CMAKE_BUILD_TYPE, shared vs static libs, fPIC) #1929

Merged
merged 24 commits into from
Mar 31, 2020

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Jan 16, 2020

This now contains (is rebased on) #1921

I worked on some more parameters in the CMakeMake EB.

  • out of source builds: separate_build_dir gets now set by EasyBlocks that require it
  • fPIC: Honor toolchainopts
  • new EasyConfig parameter build_shared_libs to unify existing practice
  • build_type honors toolchainopts (defaults to Release)

See individual commits

easybuild/easyblocks/generic/cmakemake.py Outdated Show resolved Hide resolved
easybuild/easyblocks/d/dolfin.py Outdated Show resolved Hide resolved
def extra_options():
extra_vars = CMakePythonPackage.extra_options()
extra_vars['separate_build_dir'][0] = True
extra_vars['build_type'][0] = 'Release'
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, if toolchainopts has debug True, should probably be Debug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd actually solve this in the cmakemake block: when it is none (default) then use either release or debug depending on the toolchainopt
This would be a change to current behavior where it is not currently set, but imo is the right thing as it is the equivalent of what is done for other build systems. See #911 for discussion on defaulting to release

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, doing this in cmakemake is clearly the "right" thing to do.

Note though that for dolfin you changed it from the previous Debug to Release
Although that one is kind of wrong anyway...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you are ok I'd change that in the cmakemake eb so release will become the default effectively. I'd prefer that very much

In this case this is correct as the debug was unintended: #1920

Copy link
Contributor Author

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

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

The setting of build_type depending on toolchainopts is now done in the CMakeMake EB and this line has been removed

easybuild/easyblocks/b/blender.py Outdated Show resolved Hide resolved
@Flamefire
Copy link
Contributor Author

I rebased this and included the very related PR #1921, which I now closed. The final result should now be complete and ready to go.

…Make based EBs

Some explicit settings are taken from easybuilders#616
and enhanced after testing with forced out-of-source build for all ECs using CMake
but excluding ones using the Intel compiler
Other did specify separate_build_dir in the configure step which makes EB show the wrong default value
@Flamefire Flamefire force-pushed the cmake_build_folder branch from 8931d5a to 2907090 Compare March 17, 2020 08:36
If the build_type is not set explicitely, check toolchainopts[debug] and
use either 'Debug' or 'Release'.
The influence of 'Release' is rather minor as optarch is always used
which is probably set to some optimization level already and hence
overwrites the CMake-set optimization level.
It still is a good and reasonable default similar to e.g. ConfigureMake
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.

easybuild/easyblocks/c/clang.py Outdated Show resolved Hide resolved
easybuild/easyblocks/d/dolfin.py Outdated Show resolved Hide resolved
easybuild/easyblocks/e/elsi.py Outdated Show resolved Hide resolved
easybuild/easyblocks/e/elsi.py Show resolved Hide resolved
easybuild/easyblocks/generic/cmakemake.py Outdated Show resolved Hide resolved
easybuild/easyblocks/l/lammps.py Outdated Show resolved Hide resolved
easybuild/easyblocks/o/opencv.py Show resolved Hide resolved
easybuild/easyblocks/s/superlu.py Show resolved Hide resolved
easybuild/easyblocks/generic/cmakemake.py Show resolved Hide resolved
easybuild/easyblocks/generic/cmakemake.py Outdated Show resolved Hide resolved
easybuild/easyblocks/l/lammps.py Show resolved Hide resolved
@Flamefire Flamefire force-pushed the cmake_build_folder branch from 8887196 to dc05bb8 Compare March 30, 2020 15:24
@Flamefire Flamefire force-pushed the cmake_build_folder branch from dc05bb8 to 94bfad6 Compare March 30, 2020 15:29
@Flamefire Flamefire force-pushed the cmake_build_folder branch from 94bfad6 to f4df495 Compare March 30, 2020 16:06
@boegel boegel changed the title Simplify various CMake EasyBlocks simplify various CMakeMake-based easyblocks by enhancing CMakeMake (w.r.t. CMAKE_BUILD_TYPE, shared vs static libs, fPIC) Mar 31, 2020
@boegel
Copy link
Member

boegel commented Mar 31, 2020

Did extensive review & testing on top of this (actually on top of #1933), couldn't find any more issues or breaking changes, so merging this...

Thanks a lot for your efforts on this @Flamefire!

@boegel boegel merged commit 713e5d9 into easybuilders:develop Mar 31, 2020
@Flamefire Flamefire deleted the cmake_build_folder branch April 1, 2020 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants