CMake: do release build by default #911

wpoely86 · 2016-05-09T11:15:05Z

@boegel please review

boegel · 2016-05-09T11:28:25Z

@wpoely86 hmm, do you think this is backwards compatible? Is it possible that some applications are unaware of this configure option, and that it'll break builds?

boegel · 2016-05-09T11:28:54Z

easybuild/easyblocks/generic/cmakemake.py

@@ -83,6 +84,10 @@ def configure_step(self, srcdir=None, builddir=None):
                srcdir = default_srcdir

        options = ['-DCMAKE_INSTALL_PREFIX=%s' % self.installdir]
+
+        if self.cfg.get('releasebuild', True):
+            options.append("-DCMAKE_BUILD_TYPE=Release")


what if -DCMAKE_BUILD_TYPE is already specified?

The last one will win (which means the one specified by someone else)

wouldn't it be nicer to check whether -DCMAKE_BUILD_TYPE is already specified via self.cfg['configopts']?

if self.cfg.get('releasebuild', True) and '-DDCMAKE_BUILD_TYPE' not in self.cfg['configopts']: options.append("-DCMAKE_BUILD_TYPE=Release")

in that case, you can even drop the releasebuild parameter?

also, it would be nicer to support build_type = 'Release' instead anyway?

wpoely86 · 2016-05-09T11:29:34Z

It's part of CMake for as long as I know of. It's not application specific.

hpcugentbot · 2016-05-09T11:33:48Z

Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1962/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

hpcugentbot · 2016-05-09T13:06:22Z

Easyblocks unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1963/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

hpcugentbot · 2016-05-09T13:09:56Z

Easyblocks unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1964/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

hpcugentbot · 2016-05-09T13:14:20Z

Easyblocks unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1965/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

hpcugentbot · 2016-05-09T13:18:58Z

Easyblocks unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1966/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

boegel · 2016-05-09T14:07:57Z

easybuild/easyblocks/generic/cmakemake.py

+
+        super(CMakeMake, self).__init__(*args, **kwargs)
+
+        if self.cfg['buildtype'] not in CMAKE_BUILD_TARGETS:


use self.cfg.get('buildtype', None)

wpoely86 · 2016-05-09T14:24:28Z

@boegel ready

hpcugentbot · 2016-05-09T14:26:04Z

Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1967/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

boegel · 2016-05-09T14:28:14Z

@wpoely86 strictly speaking, we should retest all easyconfigs/easyblocks that use CMakeMake... I suspect that list will be big (see also #616)

boegel · 2016-05-09T14:28:40Z

easybuild/easyblocks/generic/cmakemake.py

@@ -40,6 +40,7 @@
 from easybuild.tools.environment import setvar
 from easybuild.tools.run import run_cmd

+CMAKE_BUILD_TARGETS = ['Debug', 'Release', 'RelWithDebInfo', 'MinSizeRel']


add a docs URL for this?

boegel · 2016-05-09T14:31:20Z

easybuild/easyblocks/generic/cmakemake.py

+
+        if self.cfg['buildtype'] not in CMAKE_BUILD_TARGETS:
+            raise EasyBuildError("The specified build type for CMake is not known. Accepted values: " \
+                                 + ', '.join(CMAKE_BUILD_TARGETS))


please use this to avoid the \:

raise EasyBuildError("The specified build type for CMake is not known. Accepted values: %s", ', '.join(CMAKE_BUILD_TARGETS))

hpcugentbot · 2016-05-09T14:39:38Z

Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1968/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

hpcugentbot · 2016-05-09T14:46:32Z

Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1969/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

hpcugentbot · 2016-05-09T14:53:39Z

Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1970/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

boegel · 2016-05-10T20:29:17Z

easybuild/easyblocks/generic/cmakemake.py

@@ -51,9 +53,20 @@ def extra_options(extra_vars=None):
        extra_vars.update({
            'srcdir': [None, "Source directory location to provide to cmake command", CUSTOM],
            'separate_build_dir': [False, "Perform build in a separate directory", CUSTOM],
+            'buildtype': [CMAKE_BUILD_TYPES[0], "Build type for CMake. Accepted values: " + ', '.join(CMAKE_BUILD_TYPES),


I don't like this approach, we might miss the impact of reordering the CMAKE_BUILD_TYPES alphabetically

I prefer using CMAKE_BUILD_TYPE_RELEASE...

Flamefire · 2019-12-04T12:42:08Z

Current solution LGTM (except the erroneous bamtools inclusion). It is minimal and does "The Right Thing" by default

boegel · 2019-12-10T21:08:53Z

Current solution LGTM (except the erroneous bamtools inclusion). It is minimal and does "The Right Thing" by default

@Flamefire It does make sense, but the question remains whether a change like this is going to have fallout. Perhaps there are creative ways where using -DCMAKE_BUILD_TYPE=Release would make the installation fail (for example because the developers figured it would be a good idea to come up with their own list of known build types)...

Flamefire · 2019-12-11T08:59:54Z

The default build type (empty string) is a valid one but means only a baseline. All optimizations are usually not turned on, so the compiler will be running without any -O options. This is probably OK because EB defaults CFLAGS to include -O2. However CMake also adds e.g. -DNDEBUG to release builds to disable assertions, and probably there are also other optimizations. As EB is for HPC having not-fully-optimized libs installed can cost real money (in compute time). Of course impact depends on the actual code, but still...

While I understand your concern I don't think it will be a problem in practice. Most (if not all) CMake projects are built on CI with -DCMAKE_BUILD_TYPE=Release and/or -DCMAKE_BUILD_TYPE=Debug. I'm not sure of no build type is actually tested. Some CMake projects have code to change the empty build type to Release to mitigate this issue. Coming up with their own build types is of course possible, but usually they append to the common ones (e.g. for a profiling or sanitized build) And for the very unlikely case that there is no Release type you got the buildtype option to change it per EC.

So bottom line: IMO this should have been merged already ;-)

if @wpoely86 is no longer active, you could cherry-pick the changes and make the final adjustments or I could open a PR and mention @wpoely86 in the commit message

akesandgren · 2019-12-11T09:58:09Z

"Most (if not all) CMake projects are built on CI with -DCMAKE_BUILD_TYPE=Release and/or -DCMAKE_BUILD_TYPE=Debug"

This is, sadly, not true. Way to many projects using CMake doesn't have a clue about CMAKE_BUILD_TYPE, nor about proper optimization flags.

And there is one thing that always trumps performance... correctness of results.
And WAY to many codes are poorly written and breaks when optimization is turned up.

I.e., using Release by default is likely to break a lot of codes that we have in the tree.
So before we do that I think we should setup proper tests suites, with actual runs of the codes with real user inputs, to compare.
This is sadly very time consuming to setup...

Just to make things clear. I'm all for performance, as long as the results are correct. I've just seen too much code that breaks if you just look at it...

Flamefire · 2019-12-11T10:21:26Z

This is, sadly, not true. Way to many projects using CMake doesn't have a clue about CMAKE_BUILD_TYPE, nor about proper optimization flags.

I guess a check is due then. An easy way I'd imagine: Could EB be used to download and extract all sources and break further processing just before calling cmake ...? This will put all sources into a subfolder of a /tmp folder and one can grep for CMAKE_BUILD_TYPE and CMAKE_CONFIGURATION_TYPES (or what the name was). All projects using that are probably ok. Same for projects using CMAKE_<CXX|C>_FLAGS_RELEASE

At the least: Could we add this option with empty default? Then ECs/EBs can add it where they know

And WAY to many codes are poorly written and breaks when optimization is turned up.

Can you say which and how/why?

Counter-example: Caffee is very much aware of it and expects it to be either Release or Debug.

akesandgren · 2019-12-11T10:41:05Z

My pet project for broken code was (until I got enough patches accepted upstream) VASP. Now it's just half bad. Not using CMake though, but that wasn't my main point on that part.

I don't have any specifics at the moment, been too busy installing software on user requests and making EB better. But you can probably take any bioinformatics code as an example. Or Amber...
or QuantumESPRESSO, esp the code they have to produce the vdW_kernel table which breaks horribly when going to -O2 if i remember correctly.

And yes, there are lots of really good codes out there, like GROMACS, and for instance the blas/lapack/scalapack code. (And yes I've found more than one bug in them over the years causing them to break when increasing optimization)

And when it comes to correctness, I, personally, would not trust even code that by default uses Release, to deliver correct results...

Anyway, this is a discussion that could go on for ages, there is always new code coming out, both broken by design and good examnples.

Flamefire · 2019-12-11T12:11:21Z

But you can probably take any bioinformatics code as an example. Or Amber...
or QuantumESPRESSO, esp the code they have to produce the vdW_kernel table which breaks horribly when going to -O2 if i remember correctly.

Well, EB adds -O2 by default. So if there are broken packages included, they'll be broken already.

For current usage in EB:

There are 212 software packages using CMakeMake
36 of those use -DCMAKE_BUILD_TYPE
from a quick scan about half of them are "good CMake" and (probably) support build types (just judging names I know)
no (to me) known bad examples, like the mentioned Amber, QuantumESpresso

IMO Release should be the default just as CFLAGS=-O2 is. There is always the option to opt-out. If one can get EB to download and extract the relevant packages a grep can help filtering out "good" candidates. Depending on how many are left those can be checked or have (probably automatically) the opt-out added.

branfosj · 2023-09-10T10:46:01Z

#1929 (CMake release build by default) and #1656 (Bamtools), so closing.

CMake: do release build by default

5b75523

boegel added this to the v2.8.0 milestone May 9, 2016

boegel reviewed May 9, 2016
View reviewed changes

Rework build type for cmake

c9d02eb

Fix typo

d27eddd

Another typo

1bb746a

Another typo

2825c0c

boegel reviewed May 9, 2016
View reviewed changes

Fix error with BamTools and CMake

33a539e

boegel reviewed May 9, 2016
View reviewed changes

Fix remarks

61bef52

boegel reviewed May 9, 2016
View reviewed changes

wpoely86 added 2 commits May 9, 2016 16:32

Fix another remark

d8f79ff

More remarks

a1c16e2

boegel reviewed May 10, 2016
View reviewed changes

boegel removed this from the 3.1.0 milestone Jan 30, 2017

boegel modified the milestones: 3.2.0, 3.3.0 May 2, 2017

boegel modified the milestones: 3.3.0, 3.x Jun 22, 2017

boegel modified the milestones: 3.x, 4.x Dec 10, 2019

boegel added change enhancement labels Dec 10, 2019

This was referenced Jan 13, 2020

add custom easyconfig parameter 'buildtype' to generic CMakeMake easyblock #1915

Merged

change various easyblock deriving from CMakeMake to use 'build_type' easyconfig parameter #1921

Closed

Flamefire mentioned this pull request Feb 14, 2020

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

Merged

branfosj closed this Sep 10, 2023

wpoely86 deleted the cmake branch September 10, 2023 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake: do release build by default #911

CMake: do release build by default #911

wpoely86 commented May 9, 2016

boegel commented May 9, 2016

boegel May 9, 2016

wpoely86 May 9, 2016

boegel May 9, 2016

wpoely86 commented May 9, 2016

hpcugentbot commented May 9, 2016

hpcugentbot commented May 9, 2016

hpcugentbot commented May 9, 2016

hpcugentbot commented May 9, 2016

hpcugentbot commented May 9, 2016

boegel May 9, 2016 •

edited

Loading

wpoely86 commented May 9, 2016

hpcugentbot commented May 9, 2016

boegel commented May 9, 2016

boegel May 9, 2016

boegel May 9, 2016

hpcugentbot commented May 9, 2016

hpcugentbot commented May 9, 2016

hpcugentbot commented May 9, 2016

boegel May 10, 2016

Flamefire commented Dec 4, 2019

boegel commented Dec 10, 2019

Flamefire commented Dec 11, 2019

akesandgren commented Dec 11, 2019

Flamefire commented Dec 11, 2019

akesandgren commented Dec 11, 2019

Flamefire commented Dec 11, 2019

branfosj commented Sep 10, 2023


		super(CMakeMake, self).__init__(args, *kwargs)

		if self.cfg['buildtype'] not in CMAKE_BUILD_TARGETS:

CMake: do release build by default #911

CMake: do release build by default #911

Conversation

wpoely86 commented May 9, 2016

boegel commented May 9, 2016

boegel May 9, 2016

Choose a reason for hiding this comment

wpoely86 May 9, 2016

Choose a reason for hiding this comment

boegel May 9, 2016

Choose a reason for hiding this comment

wpoely86 commented May 9, 2016

hpcugentbot commented May 9, 2016

hpcugentbot commented May 9, 2016

hpcugentbot commented May 9, 2016

hpcugentbot commented May 9, 2016

hpcugentbot commented May 9, 2016

boegel May 9, 2016 • edited Loading

Choose a reason for hiding this comment

wpoely86 commented May 9, 2016

hpcugentbot commented May 9, 2016

boegel commented May 9, 2016

boegel May 9, 2016

Choose a reason for hiding this comment

boegel May 9, 2016

Choose a reason for hiding this comment

hpcugentbot commented May 9, 2016

hpcugentbot commented May 9, 2016

hpcugentbot commented May 9, 2016

boegel May 10, 2016

Choose a reason for hiding this comment

Flamefire commented Dec 4, 2019

boegel commented Dec 10, 2019

Flamefire commented Dec 11, 2019

akesandgren commented Dec 11, 2019

Flamefire commented Dec 11, 2019

akesandgren commented Dec 11, 2019

Flamefire commented Dec 11, 2019

branfosj commented Sep 10, 2023

boegel May 9, 2016 •

edited

Loading