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

enhance CMakeMake easyblock to run ctest command if runtest is True #2838

Merged

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Nov 29, 2022

(created using eb --new-pr)

When using the CMakeMake easyblock or a child of it like CMakeNinja you can use runtest = 'test' to run the tests which is however not user friendly and for CMakeNinja it does not work because it runs make test, so you need test_cmd

Some EasyBlocks already allow runtest = True to enable running tests, this PR introduces that for CMakeMake and defaults the test_cmd in that case to ctest and (if supported by recent CMake) appends --no-tests=error so that runtest = True will not silently succeed when no tests are found.

Requires #2837 as setting test_cmd was not enough (error when concatenating the None of runtest) and it needs a default of None for test_cmd so it is able to detect if the user set it or wants the default.

Written with backward compatibility in mind.

requires:

@boegel
Copy link
Member

boegel commented Dec 7, 2022

@Flamefire At first glance, this will cause trouble with the EB_Amber easyblock, which derives from CMakeMake and overrides runtest to set it to True by default, but doesn't override the test_step method...

There may be other easyblocks with a similar problem.

@boegel boegel changed the title Allow runtest = True for CMakeMake enhance CMakeMake easyblock to run ctest command if runtest is True Dec 7, 2022
@Flamefire
Copy link
Contributor Author

@Flamefire At first glance, this will cause trouble with the EB_Amber easyblock, which derives from CMakeMake and overrides runtest to set it to True by default, but doesn't override the test_step method...

It does: https://github.com/easybuilders/easybuild-easyblocks/blob/develop/easybuild/easyblocks/a/amber.py#L216-L218

There may be other easyblocks with a similar problem.

I'm very sure there cannot be a problem. I introduced runtest with a default of None and explicitly check for True (as opposed to true-ish). And previously a value of True without overriding the test_step would lead to an error as ConfigureMake concats that value at

so it would try to execute True which IMO is safe to assume to be an error.

@Flamefire
Copy link
Contributor Author

Anything missing here?

@boegel
Copy link
Member

boegel commented Sep 13, 2023

Anything missing here?

@Flamefire A battery of tests with existing easyconfigs that either use CMakeMake directly, or an easyblock that derives from it, mostly.

Thanks for clarifying why this shouldn't cause trouble, that helps.

@Flamefire
Copy link
Contributor Author

Test report by @Flamefire

Overview of tested easyconfigs (in order)

  • SUCCESS QtKeychain-0.13.2-GCCcore-11.2.0.eb
  • SUCCESS json-c-0.15-GCCcore-11.2.0.eb
  • SUCCESS RapidJSON-1.1.0-GCCcore-11.2.0.eb
  • SUCCESS utf8proc-2.6.1-GCCcore-11.2.0.eb
  • SUCCESS double-conversion-3.1.5-GCCcore-11.2.0.eb
  • SUCCESS freeglut-3.2.1-GCCcore-11.2.0.eb
  • SUCCESS fmt-7.1.1-GCCcore-11.2.0.eb
  • SUCCESS OpenJPEG-2.4.0-GCCcore-10.3.0.eb
  • SUCCESS libzip-1.7.3-GCCcore-10.3.0.eb
  • SUCCESS libjpeg-turbo-2.0.6-GCCcore-10.3.0.eb
  • SUCCESS json-c-0.15-GCCcore-10.3.0.eb
  • SUCCESS RapidJSON-1.1.0-GCCcore-10.3.0.eb
  • SUCCESS utf8proc-2.6.1-GCCcore-10.3.0.eb
  • SUCCESS double-conversion-3.1.5-GCCcore-10.3.0.eb
  • SUCCESS make-4.3-GCCcore-10.2.0.eb
  • SUCCESS netCDF-Fortran-4.5.3-gompi-2021b.eb
  • SUCCESS netCDF-4.7.1-gompic-2019b.eb
  • SUCCESS PnetCDF-1.12.2-gompi-2020b.eb
  • SUCCESS PnetCDF-1.12.3-gompi-2021b.eb
  • SUCCESS netCDF-Fortran-4.5.2-gompic-2019b.eb
  • FAIL (build issue) Amber-20.11-foss-2020b-AmberTools-21.3.eb (partial log available at https://gist.github.com/Flamefire/df2eaabac4355806ea6985b4847d4fa3)
  • FAIL (build issue) Amber-22.0-foss-2021b-AmberTools-22.3.eb (partial log available at https://gist.github.com/Flamefire/aebe6ac72230b417a4aa1fc24fa57c8e)
  • SUCCESS Boost.Python-1.71.0-gompic-2019b.eb
  • SUCCESS SciPy-bundle-2019.10-fosscuda-2019b-Python-2.7.16.eb
  • SUCCESS Tkinter-2.7.16-GCCcore-8.3.0.eb
  • SUCCESS matplotlib-2.2.4-fosscuda-2019b-Python-2.7.16.eb
  • FAIL (build issue) Amber-18-fosscuda-2019b-AmberTools-19-patchlevel-12-17-Python-2.7.16.eb (partial log available at https://gist.github.com/Flamefire/f457b71fa8380ba90f109dc61cdcf24d)

Build succeeded for 24 out of 27 (17 easyconfigs in total)
taurusi6180.taurus.hrsk.tu-dresden.de - Linux RHEL 7.9, x86_64, Intel(R) Xeon(R) CPU E5-2680 v3 @ 2.50GHz, Python 2.7.5
See https://gist.github.com/Flamefire/eecc1b7867c078916d06c7860410b3be for a full test report.

@Flamefire
Copy link
Contributor Author

I don't have the Amber sources so they failed, rest looks good though.

@akesandgren
Copy link
Contributor

running Amber with this eb now...

@Flamefire
Copy link
Contributor Author

running Amber with this eb now...

Note that this PR requires #2837 too although I expect for Amber it doesn't make a difference.

@akesandgren
Copy link
Contributor

Test report by @akesandgren

Overview of tested easyconfigs (in order)

  • SUCCESS Amber-22.0-foss-2021b-AmberTools-22.3.eb

Build succeeded for 1 out of 1 (1 easyconfigs in total)
b-an02.hpc2n.umu.se - Linux Ubuntu 20.04, x86_64, Intel(R) Xeon(R) CPU E5-2690 v4 @ 2.60GHz, Python 3.8.10
See https://gist.github.com/akesandgren/35af42f9f484ef9851147ef552575911 for a full test report.

Copy link
Contributor

@akesandgren akesandgren left a comment

Choose a reason for hiding this comment

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

LGTM

@akesandgren
Copy link
Contributor

Test report by @akesandgren

Overview of tested easyconfigs (in order)

  • SUCCESS Amber-22.0-foss-2021b-AmberTools-22.3.eb

Build succeeded for 1 out of 1 (1 easyconfigs in total)
b-an02.hpc2n.umu.se - Linux Ubuntu 20.04, x86_64, Intel(R) Xeon(R) CPU E5-2690 v4 @ 2.60GHz, Python 3.8.10
See https://gist.github.com/akesandgren/bcccb9a6f0910ea04a240647e915a94a for a full test report.

@akesandgren
Copy link
Contributor

Going in, thanks @Flamefire!

@akesandgren akesandgren merged commit 80f28e2 into easybuilders:develop Sep 14, 2023
47 checks passed
@Flamefire Flamefire deleted the 20221129132724_new_pr_cmakemake branch September 14, 2023 11:35
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.

None yet

3 participants