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

Make CmdCp easier to use #3549

Open
wants to merge 1 commit into
base: 5.0.x
Choose a base branch
from

Conversation

Flamefire
Copy link
Contributor

The name suggest running a command and copy files. However the commands need to be specified as a regex-map list which might be executed for every source instead of once. Often a workflow might be just ./custom_configure && ./custom_build.sh which is not easy to map to this.
Instead allow non-tuples which are considered as commands to be run. When mixing tuples and non-tuples the plain commands are run last.

This was always a concern of mine that a workflow consisting of running only a command that involves some custom install procedure was less obvious than it might be. It came up recently where an EC uses MakeCp requiring parallel = False as it replaces the make cmd by a single command. In that case it is ./configure --options and ./coconut which isn't easily applicable using CmdCp

With this change it can be:

cmds_map = [
    "./configure --option1 --option2",
    "./coconut --flag 1 --flag 2",
]

I'd say this is easier to read and understand than:

cmds_map = [
   (".*", "./configure --option1 --option2 && ./coconut --flag 1 --flag 2"),
]

Also error reporting would be more focused, i.e. either "./configure failed" or "./coconut failed"

I can port a couple easyconfigs to the new syntax in a PR to the easyconfig repo for test reports and usage examples.

The name suggest running a command and copy files.
However the commands need to be specified as a regex-map list which
might be executed for every source instead of once.
Often a workflow might be just `./custom_configure && ./custom_build.sh`
which is not easy to map to this.
Instead allow non-tuples which are considered as commands to be run.
When mixing tuples and non-tuples the plain commands are run last.
@bartoldeman bartoldeman added this to the 5.0 milestone Jan 13, 2025
Copy link
Contributor

@bartoldeman bartoldeman left a comment

Choose a reason for hiding this comment

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

LGTM

I've always found the syntax for CmdCp a little odd as well.
Another confusing bit is that %(source)s refers to the original unpatched source (i.e. it takes files from the source directory, not the unpacked archive), if you apply patches they are ignored. I found that out the hard way with
https://github.com/ComputeCanada/easybuild-easyconfigs/blob/computecanada-main/easybuild/easyconfigs/s/slurm-completion/slurm-completion-23.02.7.eb

@bartoldeman
Copy link
Contributor

A very long time ago, CmdCp had a cmd parameter:
ffe30c3
I think perhaps it would be better to reintroduce cmd for simple string commands but use cmds_map for the more complex case? @boegel may know as author of the above commit?

@boegel boegel changed the title Make CmdCp easier to use Make CmdCp easier to use Jan 13, 2025
@Flamefire
Copy link
Contributor Author

The problem is the default for cmds_map.
We could add a cmds parameter to work in the same way as here that, when set, removes the default for cmds_map but that makes the default disappear from the docstring. Can add it to the help though.

Another confusing bit is that %(source)s refers to the original unpatched source

Currently it is the file stem (name w/o extension) of the source, so not sure how it could refer to the "unpatched source". Maybe that has changed? Or I misunderstand the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants