-
Notifications
You must be signed in to change notification settings - Fork 206
Make generic EasyBlock usable to install software #4531
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
base: develop
Are you sure you want to change the base?
Conversation
…ethod to execute stack of commands
…ds and main command with options
…ny easyblock defined
…uild step of the generic EasyBlock
I need to give this a detailed review, but in general I quite like this idea to support "easyblock-less" installations. It lowers the bar to entry for people since this may avoid the need to implement a custom easyblock for simple stuff. It does perhaps also introduce a risk to encourage producing messier easyconfigs, but it's not like we don't have those already... |
A perfect use case for this PR is easybuilders/easybuild-easyconfigs#20693 |
…sable EasyBlock class
…h usable EasyBlock class
Example easyconfig using name = 'CORSIKA'
version = '77550'
homepage = "https://www.iap.kit.edu/corsika"
description = """CORSIKA (COsmic Ray SImulations for KAscade) is a program for detailed
simulation of extensive air showers initiated by high energy cosmic ray
particles. Protons, light nuclei up to iron, photons, and many other particles
may be treated as primaries."""
toolchain = {'name': 'foss', 'version': '2023a'}
toolchainopts = {'usempi': True}
download_instructions = "Sources have to be requested to the developers"
sources = [SOURCELOWER_TAR_GZ]
checksums = ['fed74c144e22deb5a7c1d2dc1f04f0100eb2732cb48665a3da49ce471a3775ee']
dependencies = [
("ROOT", "6.30.06"),
]
# custom coconut script does not recognize -j
parallel = False
# execute ./coconut manually with your own options and extract configure command from top of config.log
configure_cmd = "./configure"
configopts = "CORDETECTOR=HORIZONTAL CORTIMELIB=TIMEAUTO CORHEMODEL=QGSJETII CORLEMODEL=URQMD "
configopts += "--enable-UPWARD --enable-SLANT --enable-THIN --enable-COREAS "
configopts += "--enable-PARALLEL --with-mpirunner-lib --enable-PARALLELIB "
build_cmd = "./coconut"
buildopts = "--batch"
preinstallopts = "mkdir -p %(installdir)s/bin && "
install_cmd = "cp"
installopts = "%(builddir)s/%(namelower)s-%(version)s/run/%(namelower)s%(version)s* %(installdir)s/bin/"
sanity_check_paths = {
'files': ['bin/corsika77550Linux_QGSII_urqmd_thin_coreas_parallel'],
'dirs': []
}
moduleclass = "phys" Test report for this PR with this example easyconfig: |
self.log.debug(f"Trying to execute {unit_test_cmd} as a command for running unit tests...") | ||
res = run_shell_cmd(unit_test_cmd) | ||
if self.cfg['runtest'] is not None: | ||
self.log.deprecated("Easyconfig parameter 'runtest' is deprecated, use 'test_cmd' instead.", "6.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lexming This implies updating all easyblocks/easyconfigs to use test_cmd
instead of runtest
before we release EasyBuild 5.0, so please open an issue on that in both repos and tag it for EasyBuild 5.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what if both test_cmd
and runtest
are set?
Either we error out, or we let one overrule the other (with logging).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I suggest to not error out and just ignore runtest
if both are defined (with a warning ofc). Fixed in 22d9648
easybuild/framework/easyblock.py
Outdated
"configure", | ||
self.cfg['pre_configure_cmds'], | ||
self.cfg['preconfigopts'], | ||
self.cfg['configopts'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks kind of weird without using named options, since pre_configure_cmd
is specified after configure_cmd
, so at the very least this should become:
"configure", | |
self.cfg['pre_configure_cmds'], | |
self.cfg['preconfigopts'], | |
self.cfg['configopts'], | |
'configure', | |
pre_cmds=self.cfg['pre_configure_cmds'], | |
pre_opts=self.cfg['preconfigopts'], | |
post_opts=self.cfg['configopts'], |
Even better would be to use only named options, so also use action='self.cfg['configure_cmd']'
and action='configure'
(and making those both None
by default in run_step_main_action
and mandatory with an is None
check in there.
Also, maybe rename run_step_main_action
to run_core_step
, and only pass a string like configure
, let run_core_step
figure out the rest (to use configure_cmd
, preconfigopts
, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, it is indeed easy to construct the commands from just the step name. Done in e05a985
self.log.debug(f"Trying to execute {unit_test_cmd} as a command for running unit tests...") | ||
res = run_shell_cmd(unit_test_cmd) | ||
if self.cfg['runtest'] is not None: | ||
self.log.deprecated("Easyconfig parameter 'runtest' is deprecated, use 'test_cmd' instead.", "6.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what if both test_cmd
and runtest
are set?
Either we error out, or we let one overrule the other (with logging).
@@ -1858,83 +1860,72 @@ def det_installversion(version, toolchain_name, toolchain_version, prefix, suffi | |||
_log.nosupport('Use det_full_ec_version from easybuild.tools.module_generator instead of %s' % old_fn, '2.0') | |||
|
|||
|
|||
def get_easyblock_class(easyblock, name=None, error_on_failed_import=True, error_on_missing_easyblock=True, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remove error_on_failed_import
, that's API breakage.
At best deprecate it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we can actually break the API between major versions 😉 I re-added it with a deprecation warning nonetheless. Done in eacd6e3
raise EasyBuildError(f"Software-specific easyblock '{class_name}' not found for {name}") | ||
else: | ||
_log.debug(f"Easyblock for {class_name} not found, but ignoring it: {err}") | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep the elif error_on_failed_import
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in eacd6e3
Co-authored-by: Kenneth Hoste <kenneth.hoste@ugent.be>
Co-authored-by: Kenneth Hoste <kenneth.hoste@ugent.be>
…md that only needs step name to generate and run the core step commands
…lass and deprecate it
Test report with last commit and experimental easyconfig for CORSIKA in #4531 (comment)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lexming Ideally we also have a dedicated for this new feature, using the toy
software we use in the tests for example, with an easyconfig where we define configure_cmd
& co and we make sure that EasyBlock
class is used
pre_step_cmds = self.cfg[step_cmd_cfgs[step][0]] | ||
if pre_step_cmds: | ||
try: | ||
self._run_cmds(pre_step_cmds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we limit the usefulness of pre_configure_cmds
by running those commands separately?
Something like export FOO=bar
in pre_configure_cmds
won't work as expected because it's run in a different shell than configure_cmd
, which limits the usefulness of pre_configure_cmds
quite a bit I think?
I can see the value though (better errors when one of the commands in pre_configure_cmds
fails, for example), but we should try and make it very clear that these commands are running in a different subshell?
if self.cfg['test_cmd'] is None: | ||
self.cfg['test_cmd'] = self.cfg['runtest'] | ||
else: | ||
self.log.warning( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we print a warning here (too), to avoid that this is overlooked easily?
@@ -88,9 +88,11 @@ | |||
"to be linked in any installed binary/library", BUILD], | |||
'bitbucket_account': ['%(namelower)s', "Bitbucket account name to be used to resolve template values in source" | |||
" URLs", BUILD], | |||
'buildopts': ['', 'Extra options passed to make step (default already has -j X)', BUILD], | |||
'buildopts': ['', 'Extra options appended to build command', BUILD], | |||
'build_cmd': [None, "Main shell command used in the build step", BUILD], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also mention something like "(only used by default build step implemented by EasyBlock class)
", or something like that?
As soon as a generic/custom easyblock is used, build_cmd
will be totally ignored.
Same goes for configure
, test
, install
'postinstallcmds': [[], 'Commands to run after the install step.', BUILD], | ||
'postinstallpatches': [[], 'Patch files to apply after running the install step.', BUILD], | ||
'postinstallmsgs': [[], 'Messages to print after running the install step.', BUILD], | ||
'pre_build_cmds': ['', 'Extra commands executed before main build command', BUILD], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention that these are run in a different subshell than the build command itself (no sharing of environment), and only when there's no generic/custom easyblock is used
Same for configure
, test
, install
elif error_on_failed_import: | ||
raise EasyBuildError(f"Failed to import '{class_name}' easyblock: {err}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't belong here, in this case the easyblock doesn't exist?
This is quite a big change, it makes the base
EasyBlock
class usable by adding generic implementations of the missing configure, build and install step.This is how this works:
easyblock
will use that onename
that matches an existingeasyblock
will use that one (i.e.EB_name
)EasyBlock
Framework will define for each step of the installation the following parameters:
pre_[step]_cmds
: list of commands to execute before the main command of the step (analogue in mechanics topostinstallcmds
)pre[step]opts
: string to be prepended to main command of the step[step]_cmd
: main command of the step[step]opts
: string to append to main command of the stepEasyconfigs can then define any of the
[step]_cmd
and thenEasyBlock
will carry out that step. Otherwise the step is skipped.Motivation for this change is two-fold:
Basically
EasyBlock
becomes a CmdCmdCmd easyblock. For instance, I recently had to install something with Bazel that needed a custom./configure.py
for the configure step andbazel build
in the build step. With this approach I can do that installation without having to write a new easyblock or hacking other easyblocks not meant for this task.I initially only implemented the addition of
pre[step]cmds
parameters to execute commands at the beginning of each step. However, it is extremely weird to define those parameters in framework without any implementation. Custom easyblocks are then free to do whatever they like with those parameters. This PR adds arun_main_step_action
method that serves as a reference on how all the parameters should be combined and executed.Changelog:
pre_configure_cmds
,pre_build_cmds
,pre_test_cmds
,pre_install_cmds
configure_cmd
,build_cmd
,test_cmd
,install_cmd
easyblock._run_command_stack()
easyblock.configure_step()
usingeasyblock._run_command_stack()
easyblock.build_step()
usingeasyblock._run_command_stack()
easyblock.install_step()
usingeasyblock._run_command_stack()
easyblock.test_step()
usingeasyblock._run_command_stack()
easyblock.run_post_install_commands()
usingeasyblock._run_command_stack()
easyblock.get_easyblock_class()
behaviour as follows:easyblock
will use that onename
that matches an existingeasyblock
will use that one (i.e.EB_name
)EasyBlock
error_on_failed_import
attribute fromeasyblock.get_easyblock_class()
runtest
in favor oftest_cmd