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

update ConfigureMake easyblock to error out on unknown configure args #3025

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

update ConfigureMake easyblock to error out on unknown configure args #3025

wants to merge 1 commit into from

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Oct 25, 2023 •
edited
Loading

(created using eb --new-pr)

Fixes #157

This may cause failures in some existing EasyConfigs but actually we shouldn't have any unknown args in use and if we did that easyconfig likely needs fixing as it may not do what it is supposed to or looks like it is doing.

Requires:

As identified in #3026 this should be tested with:

@Flamefire Flamefire mentioned this pull request Oct 25, 2023
Open
@boegel boegel changed the title Error out on unknown configure args update ConfigureMake easyblock to error out on unknown configure args Oct 27, 2023
@boegel boegel added this to the 4.x milestone Oct 27, 2023
@boegel
Copy link
Member

boegel commented Oct 27, 2023

@Flamefire Since this could break existing easyconfigs, we should target this for EasyBuild 5.0 imho, which gives us some leeway (keep in mind that we don't control all easyconfigs out there).

Figuring out which easyconfigs need fixing in our central easyconfigs repository is going to be really painful, but it's an exercise we need to do anyway for EasyBuild 5.0 for various other reasons (and it's a lot less painful since we've archived a large amount of old easyconfigs in the 5.0.x branches aleady).

So, can you re-target this to 5.0.x branch?

@boegel boegel added the EasyBuild-5.0 EasyBuild 5.0 label Oct 27, 2023
@boegel boegel modified the milestones: 4.x, 5.0 Oct 27, 2023
@Flamefire Flamefire changed the base branch from develop to 5.0.x October 27, 2023 10:56
@Flamefire Flamefire force-pushed the 20231025113307_new_pr_configuremake branch from bc13247 to 660db7e Compare October 27, 2023 10:58
@Flamefire Flamefire mentioned this pull request Oct 27, 2023
Open
@Flamefire Flamefire force-pushed the 20231025113307_new_pr_configuremake branch from 660db7e to 5c72353 Compare October 27, 2023 11:12
@Flamefire
Copy link
Contributor Author

I added #3026 for the 4.x branch to (only) show a well-visible warning and included that commit here upgrading it to an error in a 2nd commit (I guess you merge develop to 5.x regularly? That should help avoiding the conflict)

@Flamefire Flamefire mentioned this pull request Jan 5, 2024
Closed
@Flamefire
Copy link
Contributor Author

@boegel I added the requested opt-out option. I felt that a bool is not enough as we might want to still show a warning but not fail the build. Hence I reused ERROR, WARN, IGNORE

@boegel
Copy link
Member

boegel commented Jan 15, 2024

@Flamefire Can you look into fixing merge conflict?

@Flamefire Flamefire force-pushed the 20231025113307_new_pr_configuremake branch from e8af859 to 9d32ba3 Compare January 15, 2024 15:54
@Flamefire
Copy link
Contributor Author

@Flamefire Can you look into fixing merge conflict?

Done. Was simply the run_shell_cmd change.

boegel
boegel requested changes Jan 22, 2024
View reviewed changes
easybuild/easyblocks/generic/configuremake.py
@@ -195,6 +196,11 @@ def extra_options(extra_vars=None):
'tar_config_opts': [False, "Override tar settings as determined by configure.", CUSTOM],
'test_cmd': [None, "Test command to use ('runtest' value is appended, default: '%s')" % DEFAULT_TEST_CMD,
CUSTOM],
'unrecognized_configure_options': [ERROR,
Copy link
Member

Choose a reason for hiding this comment

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

I would go with unknown_configure_options (it's not because configure uses "unrecognized" that we should too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to use the same terminology here as the error we check for. But can change to the shorter "unknown" if you like

easybuild/easyblocks/generic/configuremake.py Outdated Show resolved Hide resolved
easybuild/easyblocks/generic/configuremake.py
@@ -325,6 +331,24 @@ def configure_step(self, cmd_prefix=''):

res = run_shell_cmd(cmd)

action = self.cfg['unrecognized_configure_options']
Copy link
Member

Choose a reason for hiding this comment

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

action is waaay too generic, let's use unknown_opts_action or so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is only used in a very short code block right here so I'd say the short name is fine, especially with the valid_actions below which would then consequently be renamed to valid_unknown_opts_actions which I find gets to bulky

@Flamefire Flamefire force-pushed the 20231025113307_new_pr_configuremake branch from aebcc0c to 88036f2 Compare January 22, 2024 15:48
@Flamefire
Copy link
Contributor Author

As for documenting "ERROR": We already have "defaults to aborting the build" and the default is ERROR so IMO this is already sufficiently documented. The alternative "'ERROR' (the default) aborts the build with an error" or something like that sounds bulky and doesn't add much value as a) we already give the information that this is the default and b) I don't expect anyone to use this value but rather omit the EC param if that behavior is intended

@Flamefire
Copy link
Contributor Author

FYI: This found an actual issue for me: Python 3.11 moved from using configure args to env variables for TCL

@Flamefire Flamefire mentioned this pull request Feb 3, 2024
Merged
@Flamefire
Copy link
Contributor Author

With the recent updates to the EasyConfigs this can now be merged. I did an extensive testing using the current develop ECs with the identified, potentially problematic ones, see #3026 (comment)

Note that Tcl still has the configure warning in the log but it happens during make i.e. the build step. So there is some internal issue with Tcl but as it doesn't happen in the configure step it does not trigger this check. Hence it is not an issue for us.

@boegel
Copy link
Member

boegel commented Mar 11, 2024

@Flamefire I've updated the overview in the PR description, it seems like there are a couple of unresolved cases still (and some PRs that should get merged soon).

A lot of progress has been made though, it definitely looks like we'll be able to get this change included in EasyBuild v5.0 now, thanks a lot for your efforts on this!

@Flamefire
Copy link
Contributor Author

Flamefire commented Mar 11, 2024 •
edited
Loading

MESS-0.1.6-foss-2019b.eb requires SLATEC but the slatec_src.tgz is not available, at least not the right version (sha df009d9ef9c18aae06ce68711b1ae108d3533b4f174582c3cbea0915c4fdfe01)
Given the 2019b toolchain I'd let that slide. Or does anyone have that source available somewhere?

git-annex-10.20230802-GCCcore-12.2.0.eb uses MakeCp and I can't find any issue in the log

gap-4.12.2-foss-2022a.eb Shows the warning only in the build step because the internal command passes this to all dependencies and one doesn't know about it.

bigdft-suite-1.9.1-foss-2021b.eb is BigDFT and doesn't exist It was renamed in the PR that would have added it: easybuilders/easybuild-easyconfigs#15860

Yambo-5.2.dev-20230512-intel-2021b.eb must be some custom EC of yours. We only have Yambo-5.1.2-intel-2021b.eb and that installs without issues.

@Flamefire
Copy link
Contributor Author

I finished checking the list. In #3026 (comment) I also tested everything on our system that potentially had this issue and all installed fine.

Hence I'd strongly suggest to merge both PRs: The deprecation warning for 4.x and this for 5.x

@Flamefire Flamefire mentioned this pull request Mar 13, 2024
Merged
@Flamefire Flamefire force-pushed the 20231025113307_new_pr_configuremake branch from 88036f2 to 4ca608b Compare April 25, 2024 13:30
@Flamefire Flamefire requested a review from boegel April 25, 2024 13:30
@Flamefire Flamefire force-pushed the 20231025113307_new_pr_configuremake branch from 4ca608b to 0306aca Compare June 6, 2024 12:04
@Flamefire Flamefire force-pushed the 20231025113307_new_pr_configuremake branch from 460c0fb to af155f5 Compare August 8, 2024 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boegel boegel Awaiting requested review from boegel

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
change EasyBuild-5.0 EasyBuild 5.0 enhancement
Projects
EasyBuild v5.0
Status: Breaking changes
Milestone
5.0
Development

Successfully merging this pull request may close these issues.

trip over unknown configure options
2 participants
@Flamefire @boegel