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.

Sign up for GitHub

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

Jump to bottom

convert all generic easyblocks to run_shell_cmd #3046

Merged
merged 31 commits into from
Jan 6, 2024
Merged

convert all generic easyblocks to run_shell_cmd #3046

merged 31 commits into from
Jan 6, 2024

Conversation

bartoldeman
Copy link
Contributor

For ConfigureMake the default for the verbose= parameter for build_step is reversed in line with the default for run_shell_cmd.

@bartoldeman
Convert ConfigureMake easyblock to run_shell_cmd.
714bd2c 
The default for the verbose= parameter for build_step is reversed
in line with the default for run_shell_cmd.
@bartoldeman bartoldeman marked this pull request as draft December 6, 2023 13:02
@bartoldeman bartoldeman added the EasyBuild-5.0 EasyBuild 5.0 label Dec 6, 2023
@bartoldeman bartoldeman added this to the 5.0 milestone Dec 6, 2023
boegel
boegel previously requested changes Dec 6, 2023
View reviewed changes
easybuild/easyblocks/generic/configuremake.py Outdated Show resolved Hide resolved
@boegelbot
Copy link

@bartoldeman: Tests failed in GitHub Actions, see https://github.com/easybuilders/easybuild-easyblocks/actions/runs/7115210289
Output from first failing test suite run:

ERROR: test_det_cmake_version (test.easyblocks.easyblock_specific.EasyBlockSpecificTest)
Tests for det_cmake_version function provided along with CMakeMake generic easyblock.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/easybuild-easyblocks/easybuild-easyblocks/test/easyblocks/easyblock_specific.py", line 231, in test_det_cmake_version
    self.assertErrorRegex(EasyBuildError, "Failed to determine CMake version", det_cmake_version)
  File "/opt/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/easybuild/base/testing.py", line 164, in assertErrorRegex
    call(*args, **kwargs)
  File "/home/runner/work/easybuild-easyblocks/easybuild-easyblocks/easybuild/easyblocks/generic/cmakemake.py", line 66, in det_cmake_version
    cmd_res = run_shell_cmd(cmd, hidden=True)
  File "/opt/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/easybuild/tools/run.py", line 166, in cache_aware_func
    res = func(cmd, *args, **kwargs)
  File "/opt/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/easybuild/tools/run.py", line 323, in run_shell_cmd
    raise_run_shell_cmd_error(res)
  File "/opt/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/site-packages/easybuild/tools/run.py", line 149, in raise_run_shell_cmd_error
    raise RunShellCmdError(cmd_res, caller_info)
easybuild.tools.run.RunShellCmdError: Shell command 'cmake' failed!

----------------------------------------------------------------------
Ran 597 tests in 52.171s

FAILED (errors=1)
ERROR: Not all tests were successful.

bleep, bloop, I'm just a bot (boegelbot v20200716.01)
Please talk to my owner @boegel if you notice me acting stupid),
or submit a pull request to https://github.com/boegel/boegelbot fix the problem.

@boegel boegel changed the title Convert generic easyblocks to run_shell_cmd. Convert generic easyblocks to run_shell_cmd Dec 6, 2023
@bartoldeman
Deprecate 'verbose' parameter for build_step in ConfigureMake
c5b7f46
@bartoldeman
Use fail_on_error=False in det_cmake_version().
d5e54a9 
It handles errors itself already by just looking at the output,
no need also do it in run_shell_cmd here. This fixes the test
case for det_cmake_version().
@bartoldeman
Convert PythonPackage easyblock to run_shell_cmd.
a9be397
@bartoldeman
Convert ConfigureMakePythonPackage easyblock to run_shell_cmd
d7b1d46
@bartoldeman
Convert FortranPythonPackage easyblock to run_shell_cmd
5d80cd8
@bartoldeman
Convert VersionIndependentPythonPackage easyblock to run_shell_cmd
53ef067
@bartoldeman
Convert Tarball easyblock to run_shell_cmd
3b31e92
@bartoldeman
Convert PerlModule easyblock to run_shell_cmd
d7c6474
@bartoldeman
Convert RPackage easyblock to run_shell_cmd
52fa3c3
@bartoldeman
Convert Cargo easyblock to run_shell_cmd
30a907e
@bartoldeman
Convert Conda easyblock to run_shell_cmd
c039ab4
@bartoldeman
Convert GoPackage easyblock to run_shell_cmd
d89524c
@bartoldeman
Convert JuliaPackage easyblock to run_shell_cmd
816af4d
@bartoldeman
Convert OCamlPackage easyblock to run_shell_cmd
853a9f7
@bartoldeman
Convert OctavePackage easyblock to run_shell_cmd
8b4d0c3
@bartoldeman
Convert RubyGem easyblock to run_shell_cmd
a87daf2
@bartoldeman
Convert IntelBase easyblock to run_shell_cmd
bce0644
@bartoldeman
Convert Rpm easyblock to run_shell_cmd
73364a6
@bartoldeman
Copy link
Contributor Author

All generic easyblocks have been converted. Time for testing...

@bartoldeman bartoldeman marked this pull request as ready for review December 12, 2023 16:17
@boegel
Copy link
Member

boegel commented Jan 3, 2024

@SebastianAchilles Can you look into testing this a bit, as soon as easybuilders/easybuild-framework#4423 is merged?

@SebastianAchilles
Copy link
Member

Yes, I will test this PR, as soon as easybuilders/easybuild-framework#4423 is merged.

@SebastianAchilles
Copy link
Member

Test report by @SebastianAchilles

Overview of tested easyconfigs (in order)

  • SUCCESS GCC-system.eb

Build succeeded for 1 out of 1 (1 easyconfigs in total)
jscclxl1.int.jsc-clx.fz-juelich.de - Linux Rocky Linux 9.3, x86_64, Intel Xeon Processor (Cascadelake) (cascadelake), Python 3.9.18
See https://gist.github.com/SebastianAchilles/4d8f88f630344f13ce8be3286e6a10fe for a full test report.

@SebastianAchilles SebastianAchilles self-assigned this Jan 5, 2024
@SebastianAchilles
Copy link
Member

I think it makes sense to test, if possible, one EasyConfig per modified EasyBlock. To find recent EasyConfigs for an EasyBlock <EasyBlock> I used:

find . -type f | xargs grep -l "easyblock = '<EasyBlock>'" | xargs ls -rt | tail -20

I intend to test the following EasyConfigs:

EasyBlock Corresponding EasyConfig to test
Binary Nextflow-23.10.0.eb
Cargo cryptography-41.0.1-GCCcore-12.3.0.eb
Cargo Evcxr-REPL-0.14.2-GCCcore-12.2.0-Rust-1.65.0.eb
CMakeMake RapidJSON-1.1.0-20230928-GCCcore-12.3.0.eb
CmdCp Ninja-1.11.1-GCCcore-13.2.0.eb
Conda TRUST4-1.0.5.1.eb
ConfigureMake strace-6.6-GCCcore-12.2.0.eb
ConfigureMakePythonPackage MUSCLE3-0.7.0-foss-2022b.eb
FortranPythonPackage using derived EasyBlock numpy SciPy-bundle-2023.11-gfbf-2023b.eb
GoPackage SeqKit-2.3.1.eb
IntelBase using derived EasyBlock Advisor Advisor-2023.2.0.eb
JuliaPackage ResistanceGA-4.2-5-foss-2022a-R-4.2.1-Julia-1.9.2.eb
MesonNinja libGLU-9.0.3-GCCcore-13.2.0.eb
OCamlPackage no easyconfig found
OctavePackage Octave-7.1.0-foss-2021b.eb
PerlModule Bio-SearchIO-hmmer-1.7.3-GCC-11.2.0.eb
PythonPackage Pillow-10.0.0-GCCcore-12.3.0.eb
RPackage castor-1.7.11-foss-2022a.eb
RPM no easyconfig found
RubyGem Ruby-3.2.2-GCCcore-12.2.0.eb
SCons Cantera-3.0.0-foss-2023a.eb
SystemCompiler GCC-system.eb
SystemMPI dependencies not installed
Tarball VSCode-1.85.0.eb
VersionIndependentPythonPackage JUBE-2.4.1.eb
Waf no easyconfig found

@SebastianAchilles
Copy link
Member

Test report by @SebastianAchilles

Overview of tested easyconfigs (in order)

  • SUCCESS Nextflow-23.10.0.eb
  • SUCCESS cryptography-41.0.1-GCCcore-12.3.0.eb
  • SUCCESS Evcxr-REPL-0.14.2-GCCcore-12.2.0-Rust-1.65.0.eb
  • SUCCESS RapidJSON-1.1.0-20230928-GCCcore-12.3.0.eb
  • SUCCESS Ninja-1.11.1-GCCcore-13.2.0.eb
  • SUCCESS TRUST4-1.0.5.1.eb
  • SUCCESS strace-6.6-GCCcore-12.2.0.eb
  • SUCCESS MUSCLE3-0.7.0-foss-2022b.eb
  • SUCCESS SciPy-bundle-2023.11-gfbf-2023b.eb
  • SUCCESS SeqKit-2.3.1.eb
  • SUCCESS Advisor-2023.2.0.eb
  • SUCCESS ResistanceGA-4.2-5-foss-2022a-R-4.2.1-Julia-1.9.2.eb
  • SUCCESS libGLU-9.0.3-GCCcore-13.2.0.eb
  • SUCCESS Bio-SearchIO-hmmer-1.7.3-GCC-11.2.0.eb
  • SUCCESS Pillow-10.0.0-GCCcore-12.3.0.eb
  • SUCCESS castor-1.7.11-foss-2022a.eb
  • SUCCESS Ruby-3.2.2-GCCcore-12.2.0.eb
  • SUCCESS Cantera-3.0.0-foss-2023a.eb
  • SUCCESS GCC-system.eb
  • SUCCESS VSCode-1.85.0.eb
  • SUCCESS JUBE-2.4.1.eb

Build succeeded for 21 out of 21 (21 easyconfigs in total)
jscclxc1.int.jsc-clx.fz-juelich.de - Linux Rocky Linux 9.3, x86_64, Intel Xeon Processor (Cascadelake) (cascadelake), Python 3.9.18
See https://gist.github.com/SebastianAchilles/d676bcc8bde9132013cb2c0d11476d3e for a full test report.

SebastianAchilles
SebastianAchilles approved these changes Jan 6, 2024
View reviewed changes
Copy link
Member

@SebastianAchilles SebastianAchilles left a comment

Choose a reason for hiding this comment

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

lgtm

@SebastianAchilles SebastianAchilles dismissed boegel’s stale review January 6, 2024 09:24

Requested changes have been implemented.

@SebastianAchilles
Copy link
Member

Going in, thanks @bartoldeman!

@SebastianAchilles SebastianAchilles merged commit 437a74c into easybuilders:5.0.x Jan 6, 2024
28 checks passed
@easybuilders easybuilders deleted a comment from boegelbot Jan 7, 2024
boegel
boegel reviewed Jan 7, 2024
View reviewed changes
easybuild/easyblocks/generic/pythonpackage.py

abs_pylibdirs = [os.path.join(actual_installdir, pylibdir) for pylibdir in self.all_pylibdirs]
extrapath = "export PYTHONPATH=%s &&" % os.pathsep.join(abs_pylibdirs + ['$PYTHONPATH'])

cmd = self.compose_install_command(test_installdir, extrapath=extrapath)
run_cmd(cmd, log_all=True, simple=True, verbose=False)
run_shell_cmd(cmd, hidden=True)
Copy link
Member

Choose a reason for hiding this comment

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

@bartoldeman Did you intentionally hide the installation command before running the tests in PythonPackage.test_step?

Just wondering, not opposing it, sort of makes sense, but it is different from what we had before (verbose=False in run_cmd is not equivalent with hidden=True in run_shell_cmd)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like an oversight, since trace=False (now hidden=True) was used in many other places in this easyblock. I'll file a new PR for consistency.

bartoldeman added a commit to bartoldeman/easybuild-easyblocks that referenced this pull request Jan 15, 2024
@bartoldeman
pythonpackage: Remove hidden=True from run_shell_cmd for consistency
0aafc8c 
To address
easybuilders#3046 (comment)
as a followup to easybuilders#3046
@bartoldeman bartoldeman mentioned this pull request Jan 15, 2024
Merged
@boegel boegel changed the title Convert generic easyblocks to run_shell_cmd convert all generic easyblocks to run_shell_cmd Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boegel boegel boegel left review comments

@SebastianAchilles SebastianAchilles SebastianAchilles approved these changes

Assignees

@SebastianAchilles SebastianAchilles

Labels
EasyBuild-5.0 EasyBuild 5.0
Projects
EasyBuild v5.0
Status: No status
Milestone
5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@bartoldeman @boegelbot @boegel @SebastianAchilles