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

enhance ConfigureMake easyblock to error out on unknown configure options #3025

Merged
merged 2 commits into from
Dec 18, 2024
Merged

enhance ConfigureMake easyblock to error out on unknown configure options #3025

merged 2 commits into from
Dec 18, 2024

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
Closed
@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
Closed
@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.

@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
@Flamefire Flamefire force-pushed the 20231025113307_new_pr_configuremake branch from af155f5 to 1d17e41 Compare November 8, 2024 09:12
@Flamefire
Copy link
Contributor Author

@boegel Rebased again.

@boegel
use "options" rather than "arguments" in help text for unrecognized_c…
a2bc105 
…onfigure_options custom easyconfig parameter
@boegel boegel changed the title update ConfigureMake easyblock to error out on unknown configure args enhance ConfigureMake easyblock to error out on unknown configure options Dec 18, 2024
@boegel
Copy link
Member

boegel commented Dec 18, 2024

Test report by @boegel

Overview of tested easyconfigs (in order)

  • SUCCESS zlib-1.2.11-GCCcore-9.3.0.eb
  • SUCCESS time-1.9-GCCcore-11.3.0.eb
  • SUCCESS Tcl-8.6.13-GCCcore-13.2.0.eb
  • SUCCESS pkg-config-0.29.2-GCCcore-10.3.0.eb
  • SUCCESS netCDF-C++4-4.3.1-iimpi-2023b.eb
  • SUCCESS make-4.4.1-GCCcore-13.3.0.eb
  • SUCCESS libreadline-8.2-GCCcore-12.3.0.eb
  • SUCCESS hwloc-2.9.2-GCCcore-13.2.0.eb
  • SUCCESS HTSlib-1.19.1-GCC-13.2.0.eb
  • SUCCESS Autoconf-2.71-GCCcore-13.2.0.eb
  • SUCCESS SQLite-3.42.0-GCCcore-12.3.0.eb

Build succeeded for 11 out of 11 (11 easyconfigs in total)
node3569.doduo.os - Linux RHEL 8.8, x86_64, AMD EPYC 7552 48-Core Processor, Python 3.6.8
See https://gist.github.com/boegel/8b30b78f7c65fb4aae18f7cc858b0825 for a full test report.

@boegel boegel merged commit 678e0e8 into easybuilders:5.0.x Dec 18, 2024
19 checks passed
@Flamefire Flamefire deleted the 20231025113307_new_pr_configuremake branch December 18, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boegel boegel boegel approved these changes

Assignees
No one assigned
Labels
change EasyBuild-5.0 EasyBuild 5.0 enhancement
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.

trip over unknown configure options
2 participants
@Flamefire @boegel