search

easybuilders / easybuild-easyblocks

Collection of easyblocks that implement support for building and installing software with EasyBuild.
https://easybuild.io
GNU General Public License v2.0
106 stars 285 forks source link

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

Closed bedroge closed 2 weeks ago

bedroge commented 2 weeks ago

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 commented 2 weeks ago

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

https://github.com/easybuilders/easybuild-easyblocks/blob/80c3995b46f6b3262283d810c060731e2cce865a/easybuild/easyblocks/s/score_p.py#L146

this fix might be sufficient enough for now

bedroge commented 2 weeks ago

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?

Thyre commented 2 weeks ago

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 commented 2 weeks ago

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 ;; #(
   *) :
bedroge commented 2 weeks ago

Thanks @Thyre , I was clearly too hasty here... :sweat_smile: 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.

Thyre commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

Looks reasonable to me.

bedroge commented 2 weeks ago

Test report by @bedroge

Overview of tested easyconfigs (in order)

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 commented 2 weeks ago

@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 commented 2 weeks ago

@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!

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 commented 2 weeks ago

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 https://github.com/EESSI/software-layer/pull/797, and all builds succeeded :tada:

boegelbot commented 2 weeks ago

Test report by @boegelbot

Overview of tested easyconfigs (in order)

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 commented 2 weeks ago

The failure in https://github.com/easybuilders/easybuild-easyblocks/pull/3496#issuecomment-2444256068 is due to a failed download of PDT and not relevant to this PR