fix check in Score-P's configure scripts that may fail if the path to certain dependencies include `yes` or `no` #3496

bedroge · 2024-10-29T08:43:04Z

This prevents issues with dependencies located in paths containing yes or no, see:
https://gitlab.com/score-p/scorep/-/issues/1008

I'm restricting this to versions 8.0-8.4, as @Thyre mentioned that it will probably be fixed soon, and version 7 doesn't seem to have this check.

Thyre · 2024-10-29T09:38:56Z

Thanks a lot for the PR. The fix certainly goes in the correct direction, but doesn't fully fix the issue. With the proposed change, passing yes or no to both --with-libbfd-include and --with-libbfd-lib will not trigger the error anymore, as both values are concatenated. Errors will then likely occur later on during the actual build process.

This is the broken check in Score-P 8.x (https://gitlab.com/score-p/scorep/-/blob/b84a3703563b61d99312169b4f6a6cef1c050005/build-config/common/m4/afs_external_lib.m4#L209):

AS_CASE([${_afs_lib_withval_lib}${_afs_lib_withval_include}],
    [*yes*|*no*], [AC_MSG_ERROR([Both, --with-_afs_lib_name-lib and --with-_afs_lib_name-include require a <path>.])],
[...]

our current idea is to replace this with the following block (not yet tested):

[AS_CASE([${_afs_lib_withval_lib},${_afs_lib_withval_include}],
    [yes,*|no,*|*,yes|*,no], [AC_MSG_ERROR([Both, --with-_afs_lib_name-lib and --with-_afs_lib_name-include require a <path>.])],
[...]

This should fix the issue reported in https://gitlab.com/score-p/scorep/-/issues/1008.

Fixing this in the EasyBlock will require a few more replacements aside from the one for *yes*|*no* unfortunately, as three libraries (libbfd, libunwind, liblustre) are affected by this.

However, given that the EasyBlock always tries to pass full paths for the dependencies

easybuild-easyblocks/easybuild/easyblocks/s/score_p.py

Line 146 in 80c3995

'binutils': ['--with-libbfd-include=%s/include',

this fix might be sufficient enough for now

bedroge · 2024-10-29T10:46:21Z

Oh, right, I overlooked that the values are concatenated. Thanks! I'll change the regex and replace it by the one you provided (yes,*|no,*|*,yes|*,no).

Fixing this in the EasyBlock will require a few more replacements aside from the one for yes|no unfortunately, as three libraries (libbfd, libunwind, liblustre) are affected by this.

The current regex should replace all *yes*|*no* occurrences in all build*/configure scripts, do you mean that doesn't cover everything? From what I can tell, these scripts (e.g. build-mpi/configure) does indeed seem to have the same check for all three libraries, and hence the regex should fix them all in the same way?

…ues are concatened

Thyre · 2024-10-29T11:47:54Z

The current regex should replace all yes|no occurrences in all build*/configure scripts, do you mean that doesn't cover everything?

There's a very tiny additional difference, which is needed for the check to succeed:

- AS_CASE([${_afs_lib_withval_lib}${_afs_lib_withval_include}],
+ AS_CASE([${_afs_lib_withval_lib},${_afs_lib_withval_include}],

In the resulting configure, this means that the check for libbfd, libunwind and liblustre changes slightly as well (this is from master, therefore NVTX shows up as well):

--- build-backend/configure.old 2024-10-29 12:44:11.495387487 +0100
+++ build-backend/configure     2024-10-29 12:44:40.043430447 +0100
@@ -37729,8 +37729,8 @@
 fi ;;
 esac ;; #(
       set2set3) :
-    case ${with_libbfd_lib}${with_libbfd_include} in #(
-  *yes*|*no*) :
+    case ${with_libbfd_lib},${with_libbfd_include} in #(
+  yes,*|no,*|*,yes|*,no) :
     as_fn_error $? "Both, --with-libbfd-lib and --with-libbfd-include require a <path>." "$LINENO" 5 ;; #(
   *) :
 
@@ -46862,8 +46862,8 @@
 fi ;;
 esac ;; #(
       set2set3) :
-    case ${with_libunwind_lib}${with_libunwind_include} in #(
-  *yes*|*no*) :
+    case ${with_libunwind_lib},${with_libunwind_include} in #(
+  yes,*|no,*|*,yes|*,no) :
     as_fn_error $? "Both, --with-libunwind-lib and --with-libunwind-include require a <path>." "$LINENO" 5 ;; #(
   *) :
 
@@ -53492,8 +53492,8 @@
 fi ;;
 esac ;; #(
       set2set3) :
-    case ${with_libnvToolsExt_lib}${with_libnvToolsExt_include} in #(
-  *yes*|*no*) :
+    case ${with_libnvToolsExt_lib},${with_libnvToolsExt_include} in #(
+  yes,*|no,*|*,yes|*,no) :
     as_fn_error $? "Both, --with-libnvToolsExt-lib and --with-libnvToolsExt-include require a <path>." "$LINENO" 5 ;; #(
   *) :
 
@@ -57476,8 +57476,8 @@
 fi ;;
 esac ;; #(
       set2set3) :
-    case ${with_liblustreapi_lib}${with_liblustreapi_include} in #(
-  *yes*|*no*) :
+    case ${with_liblustreapi_lib},${with_liblustreapi_include} in #(
+  yes,*|no,*|*,yes|*,no) :
     as_fn_error $? "Both, --with-liblustreapi-lib and --with-liblustreapi-include require a <path>." "$LINENO" 5 ;; #(
   *) :

From what I can tell, these scripts (e.g. build-mpi/configure) does indeed seem to have the same check for all three libraries, and hence the regex should fix them all in the same way?

That's true. We call the same check for build-mpi and build-shmem. If we add the , as well, everything should work.

Thyre · 2024-10-29T11:57:40Z

This additional change should be sufficient to add the ,:

$ diff -u build-backend/configure.old build-backend/configure
$ sed -i 's/_lib}${with_/_lib},${with_/g' build-*/configure
$ diff -u build-backend/configure.old build-backend/configure    
--- build-backend/configure.old	2024-10-29 12:44:11.495387487 +0100
+++ build-backend/configure	2024-10-29 12:57:00.940559462 +0100
@@ -37729,7 +37729,7 @@
 fi ;;
 esac ;; #(
       set2set3) :
-    case ${with_libbfd_lib}${with_libbfd_include} in #(
+    case ${with_libbfd_lib},${with_libbfd_include} in #(
   *yes*|*no*) :
     as_fn_error $? "Both, --with-libbfd-lib and --with-libbfd-include require a <path>." "$LINENO" 5 ;; #(
   *) :
@@ -46862,7 +46862,7 @@
 fi ;;
 esac ;; #(
       set2set3) :
-    case ${with_libunwind_lib}${with_libunwind_include} in #(
+    case ${with_libunwind_lib},${with_libunwind_include} in #(
   *yes*|*no*) :
     as_fn_error $? "Both, --with-libunwind-lib and --with-libunwind-include require a <path>." "$LINENO" 5 ;; #(
   *) :
@@ -53492,7 +53492,7 @@
 fi ;;
 esac ;; #(
       set2set3) :
-    case ${with_libnvToolsExt_lib}${with_libnvToolsExt_include} in #(
+    case ${with_libnvToolsExt_lib},${with_libnvToolsExt_include} in #(
   *yes*|*no*) :
     as_fn_error $? "Both, --with-libnvToolsExt-lib and --with-libnvToolsExt-include require a <path>." "$LINENO" 5 ;; #(
   *) :
@@ -57476,7 +57476,7 @@
 fi ;;
 esac ;; #(
       set2set3) :
-    case ${with_liblustreapi_lib}${with_liblustreapi_include} in #(
+    case ${with_liblustreapi_lib},${with_liblustreapi_include} in #(
   *yes*|*no*) :
     as_fn_error $? "Both, --with-liblustreapi-lib and --with-liblustreapi-include require a <path>." "$LINENO" 5 ;; #(
   *) :

…ther

bedroge · 2024-10-29T12:36:26Z

Thanks @Thyre , I was clearly too hasty here... 😅 I've added the additional regex, and also added a fix for the other one, as it didn't completely do the right thing.

I've just done a build with the latest changes, and got the following diff for these two configure scripts:

$ diff /data/eb/build/ScoreP/8.4/gompi-2023b/scorep-8.4/build-mpi/configure.orig.eb /data/eb/build/ScoreP/8.4/gompi-2023b/scorep-8.4/build-mpi/configure
35715,35716c35715,35716
<     case ${with_libbfd_lib}${with_libbfd_include} in #(
<   *yes*|*no*) :
---
>     case ${with_libbfd_lib},${with_libbfd_include} in #(
>   yes,*|no,*|*,yes|*,no) :
45108,45109c45108,45109
<     case ${with_libunwind_lib}${with_libunwind_include} in #(
<   *yes*|*no*) :
---
>     case ${with_libunwind_lib},${with_libunwind_include} in #(
>   yes,*|no,*|*,yes|*,no) :
54091,54092c54091,54092
<     case ${with_liblustreapi_lib}${with_liblustreapi_include} in #(
<   *yes*|*no*) :
---
>     case ${with_liblustreapi_lib},${with_liblustreapi_include} in #(
>   yes,*|no,*|*,yes|*,no) :


$ diff /data/eb/build/ScoreP/8.4/gompi-2023b/scorep-8.4/build-shmem/configure.orig.eb /data/eb/build/ScoreP/8.4/gompi-2023b/scorep-8.4/build-shmem/configure
35526,35527c35526,35527
<     case ${with_libbfd_lib}${with_libbfd_include} in #(
<   *yes*|*no*) :
---
>     case ${with_libbfd_lib},${with_libbfd_include} in #(
>   yes,*|no,*|*,yes|*,no) :
44732,44733c44732,44733
<     case ${with_libunwind_lib}${with_libunwind_include} in #(
<   *yes*|*no*) :
---
>     case ${with_libunwind_lib},${with_libunwind_include} in #(
>   yes,*|no,*|*,yes|*,no) :
53701,53702c53701,53702
<     case ${with_liblustreapi_lib}${with_liblustreapi_include} in #(
<   *yes*|*no*) :
---
>     case ${with_liblustreapi_lib},${with_liblustreapi_include} in #(
>   yes,*|no,*|*,yes|*,no) :

I think this is the right result?

Edit: let me also change the build-*, maybe that's a bit too aggressive if we only need to change the ones in build-mpi and build-shmem.

…shmem

Thyre · 2024-10-29T12:40:49Z

This seems to look good now, yeah. Any objections @cfeld, @geimer?

Edit: let me also change the build-*, maybe that's a bit too aggressive if we only need to change the ones in build-mpi and build-shmem?

We need to touch build-backend, build-mpi and build-shmem, since all three do the checks. But I agree, it might be a bit too aggressive to touch all other build directories, even though it shouldn't cause any issues.

I'll do some test installations later on our systems if I find the time, hopefully today.

bedroge · 2024-10-29T12:45:14Z

Also applied the same regex to build-backend/configure, and a test build now shows the following diff for that one:

$ diff /data/eb/build/ScoreP/8.4/gompi-2023b/scorep-8.4/build-backend/configure.orig.eb /data/eb/build/ScoreP/8.4/gompi-2023b/scorep-8.4/build-backend/configure
37758,37759c37758,37759
<     case ${with_libbfd_lib}${with_libbfd_include} in #(
<   *yes*|*no*) :
---
>     case ${with_libbfd_lib},${with_libbfd_include} in #(
>   yes,*|no,*|*,yes|*,no) :
46770,46771c46770,46771
<     case ${with_libunwind_lib}${with_libunwind_include} in #(
<   *yes*|*no*) :
---
>     case ${with_libunwind_lib},${with_libunwind_include} in #(
>   yes,*|no,*|*,yes|*,no) :
56921,56922c56921,56922
<     case ${with_liblustreapi_lib}${with_liblustreapi_include} in #(
<   *yes*|*no*) :
---
>     case ${with_liblustreapi_lib},${with_liblustreapi_include} in #(
>   yes,*|no,*|*,yes|*,no) :

cfeld · 2024-10-29T12:48:31Z

This seems to look good now, yeah. Any objections @cfeld, @geimer?

Looks reasonable to me.

bedroge · 2024-10-29T13:03:41Z

Test report by @bedroge

Overview of tested easyconfigs (in order)

SUCCESS Score-P-8.4-gompi-2023b.eb

Build succeeded for 1 out of 1 (1 easyconfigs in total)
bob-Latitude-5300 - Linux Ubuntu 24.04.1 LTS (Noble Numbat), x86_64, Intel(R) Core(TM) i7-8665U CPU @ 1.90GHz, Python 3.12.3
See https://gist.github.com/bedroge/a0f3ff6820a494c896326ba3d8147db7 for a full test report.

bedroge · 2024-10-29T13:08:12Z

@boegelbot please test @ generoso
EB_ARGS="--installpath /tmp/pr3496 Score-P-8.0-gompi-2022a.eb Score-P-8.1-gompi-2023a.eb Score-P-8.3-gompi-2022b.eb Score-P-8.4-gompi-2024a.eb"

boegelbot · 2024-10-29T13:10:06Z

@bedroge: Request for testing this PR well received on login1

PR test command 'EB_PR=3496 EB_ARGS="--installpath /tmp/pr3496 Score-P-8.0-gompi-2022a.eb Score-P-8.1-gompi-2023a.eb Score-P-8.3-gompi-2022b.eb Score-P-8.4-gompi-2024a.eb" EB_CONTAINER= EB_REPO=easybuild-easyblocks /opt/software/slurm/bin/sbatch --job-name test_PR_3496 --ntasks=4 ~/boegelbot/eb_from_pr_upload_generoso.sh' executed!

exit code: 0
output:

Submitted batch job 14591

Test results coming soon (I hope)...

- notification for comment with ID 2444177078 processed

Message to humans: this is just bookkeeping information for me,
it is of no use to you (unless you think I have a bug, which I don't).

bedroge · 2024-10-29T13:34:52Z

Tested on a whole bunch of CPUs (including three Arm CPUs, all of them have no in the libbfd paths and were affected by the issue) in EESSI/software-layer#797, and all builds succeeded 🎉

boegelbot · 2024-10-29T13:36:16Z

Test report by @boegelbot

Overview of tested easyconfigs (in order)

SUCCESS Score-P-8.0-gompi-2022a.eb
SUCCESS Score-P-8.1-gompi-2023a.eb
SUCCESS Score-P-8.3-gompi-2022b.eb
FAIL (build issue) PDT-3.25.2-GCCcore-13.3.0.eb (partial log available at https://gist.github.com/boegelbot/3046ebf96bf25f5c0c044dc6fba7d89c)
FAIL (build issue) Score-P-8.4-gompi-2024a.eb (partial log available at https://gist.github.com/boegelbot/5fe22bbb21ca20b7eee4a762b35482d5)

Build succeeded for 3 out of 5 (4 easyconfigs in total)
cns1 - Linux Rocky Linux 8.9, x86_64, Intel(R) Xeon(R) CPU E5-2667 v3 @ 3.20GHz (haswell), Python 3.6.8
See https://gist.github.com/boegelbot/e6ab62ff7edad32300be8b3096217791 for a full test report.

ocaisa

LGTM

ocaisa · 2024-10-29T13:44:03Z

The failure in #3496 (comment) is due to a failed download of PDT and not relevant to this PR

specifically check for yes|no in configure scripts

6b7efa9

bedroge added bug fix EESSI Related to EESSI project labels Oct 29, 2024

remove duplicate import

80c3995

use proper fix for the yes|no check, taking into account that the val…

f23d154

…ues are concatened

add regex that adds a comma between the 'with_*' variables, fix the o…

82c5098

…ther

only apply the regex on the configure scripts in build-mpi and build-…

3c9d602

…shmem

also apply the regex to build-backend/configure

da3593f

modify comment to better reflect what this fix does

60633b0

bedroge changed the title ~~specifically check for yes|no instead of *yes*|*no* in Score-P's configure scripts~~ fix check in Score-P's configure scripts that may fail if the path to certain dependencies include yes or no Oct 29, 2024

ocaisa approved these changes Oct 29, 2024

View reviewed changes

ocaisa merged commit 2ab3cbc into easybuilders:develop Oct 29, 2024
41 checks passed

bedroge deleted the score_p_configure_yes_no_check_fix branch October 29, 2024 13:46

boegel added this to the release after 4.9.4 milestone Nov 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix check in Score-P's configure scripts that may fail if the path to certain dependencies include `yes` or `no` #3496

fix check in Score-P's configure scripts that may fail if the path to certain dependencies include `yes` or `no` #3496

bedroge commented Oct 29, 2024

Thyre commented Oct 29, 2024 •

edited

Loading

bedroge commented Oct 29, 2024

Thyre commented Oct 29, 2024 •

edited

Loading

Thyre commented Oct 29, 2024 •

edited

Loading

bedroge commented Oct 29, 2024 •

edited

Loading

Thyre commented Oct 29, 2024

bedroge commented Oct 29, 2024

cfeld commented Oct 29, 2024

bedroge commented Oct 29, 2024

bedroge commented Oct 29, 2024

boegelbot commented Oct 29, 2024

bedroge commented Oct 29, 2024

boegelbot commented Oct 29, 2024

ocaisa left a comment

ocaisa commented Oct 29, 2024

fix check in Score-P's configure scripts that may fail if the path to certain dependencies include yes or no #3496

fix check in Score-P's configure scripts that may fail if the path to certain dependencies include yes or no #3496

Conversation

bedroge commented Oct 29, 2024

Thyre commented Oct 29, 2024 • edited Loading

bedroge commented Oct 29, 2024

Thyre commented Oct 29, 2024 • edited Loading

Thyre commented Oct 29, 2024 • edited Loading

bedroge commented Oct 29, 2024 • edited Loading

Thyre commented Oct 29, 2024

bedroge commented Oct 29, 2024

cfeld commented Oct 29, 2024

bedroge commented Oct 29, 2024

Overview of tested easyconfigs (in order)

bedroge commented Oct 29, 2024

boegelbot commented Oct 29, 2024

bedroge commented Oct 29, 2024

boegelbot commented Oct 29, 2024

Overview of tested easyconfigs (in order)

ocaisa left a comment

Choose a reason for hiding this comment

ocaisa commented Oct 29, 2024

fix check in Score-P's configure scripts that may fail if the path to certain dependencies include `yes` or `no` #3496

fix check in Score-P's configure scripts that may fail if the path to certain dependencies include `yes` or `no` #3496

Thyre commented Oct 29, 2024 •

edited

Loading

Thyre commented Oct 29, 2024 •

edited

Loading

Thyre commented Oct 29, 2024 •

edited

Loading

bedroge commented Oct 29, 2024 •

edited

Loading