Skip to content

Update rivet.sh - set 3rd party paths via script, not _ROOT env#5264

Merged
maireiphc merged 4 commits into
alisw:masterfrom
cholmcc:patch-3
Dec 3, 2023
Merged

Update rivet.sh - set 3rd party paths via script, not _ROOT env#5264
maireiphc merged 4 commits into
alisw:masterfrom
cholmcc:patch-3

Conversation

@cholmcc
Copy link
Copy Markdown
Contributor

@cholmcc cholmcc commented Nov 29, 2023

See #5245

jackal1-66
jackal1-66 previously approved these changes Nov 29, 2023
@maireiphc
Copy link
Copy Markdown
Contributor

Hello @cholmcc,

thanks for your investigation on such matters !
The issues are quite nested.
I have read your explanation on #5245
I confess that I am not mastering the logic of your patch proposed here with side bash script to be executed in due time.

I mean,
take for instance YODA_ROOT needed in the recipe rivet.sh

  • Yoda is mentioned in the header of rivet.sh as both run-time and build dependencies (i.e. listed after requires ... and in fact also among build_requires)
    • So the Yoda module should loaded at run time before Rivet module is loaded.
    • In the module environment of YODA, there is var YODA_ROOT that is defined, https://github.com/alisw/alidist/blob/master/yoda.sh#L127, so I would expect such a var to be known and passed over to Rivet at run-time when one enters the Rivet module, i.e. load beforehand the yoda module, no ?

What I would have foreseen:

  1. At build time:
  • substitute in the config command ./configure ... -with-yoda="$YODA_ROOT" ... with lines like ./configure ... -with-yoda=$(yoda-config --prefix) (l.56 + l.66)
  • Similarly for HEPMC3_ROOT, FASTJET_ROOT, CGAL_ROOT, GMP_ROOT ... = Giulio's suggestion
  1. At run time, in the module part of the recipe:

Currently I get that your side bash script 3rdparty.sh embedded into the other bash script source3rd is created at some point of build time (l.117).

  • it has no consequence on the compilation ($YODA_ROOT is still provided by the module part of YODA recipe, no ?).
  • The goal is to insert the script content in the hard copy of $INSTALLROOT/bin/rivet-config (l.123) and $INSTALLROOT/bin/rivet-build (l.132) on the build machine, hard copy that will be shipped to CVMFS so made available at run time.
  • So at run time, when invoking rivet-build, the proper environment variables are expected to be picked up properly. But I do not get what is launched then as instruction on run-time machine (test -f \$prefix/etc/3rdparty.sh && source \$prefix/etc/3rdparty.sh\ ?)

Following what we would have done previously (like for alisw/rivet/3.1.6), we would have patched the rivet source 3.1.8 under alisw to arrive to the desired situation.
See for 3.1.6 the patch ported by Antonio: https://github.com/alisw/rivet/blame/bcf8d9552c2ffd9171717d961cb7b4d931f6ae84/bin/rivet-build.in
It is any good if we can avoid ALICE-made patches, but I miss a part of the logic that is necessary to fix things at the recipe level.

Sorry, I may be slow to learn on such matters.

Whoops, `grep`'ed  for `cgal` where I wanted to grep for `GMP`.
@cholmcc
Copy link
Copy Markdown
Contributor Author

cholmcc commented Nov 29, 2023

Hi Antonin,

In the module environment of YODA, there is var YODA_ROOT that is defined,
https://github.com/alisw/alidist/blob/master/yoda.sh#L127, so I would expect such a var
to be known and passed over to Rivet at run-time when one enters the Rivet module,
i.e. load beforehand the yoda module, no ?

Yes, if the run-time module defines YODA_ROOT, then it will be available when loading, at run-time, the Rivet module. However, as I understand Guilio (@ktf), defining too many environment variables as run-time is discouraged unless really needed.

Build time

  • substitute in the config command ./configure ... -with-yoda="$YODA_ROOT" ... with lines like
    ./configure ... -with-yoda=$(yoda-config --prefix) (l.56 + l.66)
  • Similarly for HEPMC3_ROOT, FASTJET_ROOT, CGAL_ROOT, GMP_ROOT ... = Giulio's suggestion

I don't think that was Guilio's suggestion:

The safest way to do that at runtime would be to modify rivet-config to do use:...

However, looking at the comment again, I see

You should not use the _ROOT variables, those are not guaranteed to be there, e.g. in the case you want
aliBuild pick up cgal / fastjet / hepmc from the system ...

so I guess you are also right. It's a good point that the recipe should in some sense be independent of aliBuild and of specific environment variables that are only defined there. That would mean changing the rivet.sh recipe - build part - to something like

cat <<EOF > 3rdparty.sh
# Define locations of 3rd party software
HEPMC3_ROOT=\`HepMC3-config --prefix\` 
YODA_ROOT=\`yoda-config --prefix\` 
FASTJET_ROOT=\`fastjet-config --prefix\` 
CGAL_ROOT=\$(dirname \$(dirname \`which cgal_create_CMakeLists\`))
GMP_ROOT=\$(dirname \`echo \$LD_LIBRARY_PATH| tr ':' '\n' | grep GMP\`)
EOF

source 3rdparty.sh 

CGAL_LDFLAGS="-L${CGAL_ROOT}/lib"
GMP_LDFLAGS="-L${GMP_ROOT}/lib"
LOCAL_LDFLAGS="${CGAL_LDFLAGS} ${GMP_LDFLAGS}"
case $ARCHITECTURE in
    osx*)
	./configure --prefix="$INSTALLROOT"            \
		    --disable-silent-rules             \
		    --disable-doxygen                  \
		    --with-yoda="$YODA_ROOT"           \
		    --with-hepmc3="$HEPMC3_ROOT"       \
		    --with-fastjet="$FASTJET_ROOT"     \
		    LDFLAGS="${LOCAL_LDFLAGS}"
	;;
    *)
	LOCAL_LDFLAGS="${LOCAL_LDFLAGS} -Wl,--no-as-needed"
	./configure --prefix="$INSTALLROOT"    	                        \
		    --disable-silent-rules                              \
		    --disable-doxygen                  			\
		    --with-yoda="$YODA_ROOT"           			\
		    --with-hepmc3="$HEPMC3_ROOT"       			\
		    --with-fastjet="$FASTJET_ROOT"     			\
		    LDFLAGS="${LOCAL_LDFLAGS}"                          \
		    CYTHON="$PYTHON_MODULES_ROOT/share/python-modules/bin/cython"
  ;;
esac

and then later on, after make install

cp 3rdparty.sh $INSTALLROOT/etc/3rdparty.sh

I.e., use the same approach (and script 😄) at both build- and run-time.

Currently I get that your side bash script 3rdparty.sh embedded into the other bash script
source3rd is created at some point of build time (l.117).

source3rd is a temporary file which isn't installed. I use it when I patch up rivet-config and rivet-build:

cat $INSTALLROOT/bin/rivet-config | sed -e "$SED_EXPR" > $INSTALLROOT/bin/rivet-config.0
csplit $INSTALLROOT/bin/rivet-config.0 '/^datarootdir=/+1'
cat xx00 source3rd xx01 >  $INSTALLROOT/bin/rivet-config
chmod 0755 $INSTALLROOT/bin/rivet-config

That is, I

  • Replace all fixed paths to GMP, CGAL, HEPMC3, YODA, FASTJET in the script rivet-config with calls to variables ${..._ROOT}
  • I split the newly generated script at the line datarootdir= - creating two files xx00 and xx01
  • Then I concatenate xx00, source3rd (which sources the installed 3rdparty.sh), and xx01, effectively adding the line source ${prefix}/etc/3rdparty.sh after the line datarootdir=... in the script rivet-config
  • similar for rivet-build

The installed script snippet 3rdparty.sh deduces the ..._ROOT variables using scripts or location of scripts, or the environment variable LD_LIBRARY_PATH.

it has no consequence on the compilation ($YODA_ROOT is still provided by the module part of YODA recipe, no ?).

Correct, as it is now. However, see also above for how to make the recipe even more robust.

The goal is to insert the script content in the hard copy of $INSTALLROOT/bin/rivet-config (l.123)
and $INSTALLROOT/bin/rivet-build (l.132) on the build machine, hard copy that will be shipped to
CVMFS so made available at run time.

The goal is not to "insert the script [3rdparty.sh]" into the installed rivet-config and rivet-build, but rather that those two scripts source the same script snippet from $prefix/etc/3rdparty.sh ($prefix is equal to the installation prefix on the run-time machine). This creates a bit of flexibility in that $prefix/etc/3rdparty.sh

  • is used by both rivet-config and rivet-build, thus ensuring some consistency.
  • can in principle be changed in a user installation to do something different or in addition to what is there.

Of course, the content of $prefix/etc/3rdparty.sh could be put directly into rivet-config and rivet-build

cat xx00 3rdparty.sh xx01 > $INSTALLROOT/bin/rivet-config 

but I think I like the flexibility of a separate script snippet.

So at run time, when invoking rivet-build, the proper environment variables are expected to be
picked up properly.

Partially correct. Instead of at run-time relying on ..._ROOT to be set before-hand, the script snippet 3rdparty.sh deduces them (the ..._ROOT variables) from scripts etc.

But I do not get what is launched then as instruction on run-time machine
(test -f $prefix/etc/3rdparty.sh && source $prefix/etc/3rdparty.sh\ ?)

This is the line that is added to rivet-config and rivet-build to source the script snippet that will deduce the ..._ROOT variables.

Perhaps it is worth to stipulate that rivet-config and rivet-build uses the ..._ROOT variables - not hard-coded paths.

Following what we would have done previously (like for alisw/rivet/3.1.6), we would have patched the
rivet source 3.1.8 under alisw to arrive to the desired situation.

Yes, that's one way to go about it. However, here I prefer to more-or-less Monkey-patch, which allows us to use upstream code and only change the things that we absolutely need.

See for 3.1.6 the patch ported by Antonio:
https://github.com/alisw/rivet/blame/bcf8d9552c2ffd9171717d961cb7b4d931f6ae84/bin/rivet-build.in

What I do, especially following Giulio's suggestion, is actually a step better than that. In that patching up, Antonio relied on specific environment variables (FASTJET, HEPMC, ...). What we have now, at least run-time, and, when I do the fix-up, for build-time, is that the proper settings are deduced from specific applications that must be present iff the 3rd party software was installed (well, with the exception of GMP, but that's OK as the alternative is often system installs).

It is any good if we can avoid ALICE-made patches, but I miss a part of the logic that is necessary to fix things
at the recipe level.

You are right in that at build-time, there's still a dependency on the ..._ROOT environment variables (I will fix that according to the above outline), but at run-time all is good (if it works - it should be tested once the merge is done 😄).

Sorry, I may be slow to learn on such matters.

No worries - I do play a few tricks, but not hazardly 😄

Yours,
Christian

Christian Holm Christensen added 2 commits November 29, 2023 20:48
Following discussion on this MR, I made some changes so that at build-time, we deduce variables in same way as done at run-time (uses the same script snippet). 

The script snippet has been made more robust, and will exit out (build, rivet-config, rivet-build) in case of problems. 

If variables are set before hand (e.g., `HEPMC3_ROOT` is already set), then that value is used as-is.
@cholmcc
Copy link
Copy Markdown
Contributor Author

cholmcc commented Nov 29, 2023

The failure of build/AliGenerators/generators is real, though bewildering

++ source rivet_3rdparty.sh
+++ hash HepMC3-config
+++ test x/sw/slc7_x86-64/HepMC3/3.2.5-100 = x
+++ hash yoda-config
+++ test x/sw/slc7_x86-64/YODA/yoda-1.9.7-3 = x
+++ hash fastjet-config
+++ test x/sw/slc7_x86-64/fastjet/v3.4.1_1.052-alice2-4 = x
+++ hash cgal_create_CMakeLists
+++ test x/sw/slc7_x86-64/cgal/4.6.3-129 = x
+++ test x/sw/slc7_x86-64/GMP/v6.2.1-43 = x
+++ test x/sw/slc7_x86-64/HepMC3/3.2.5-100 = x
+++ test x/sw/slc7_x86-64/YODA/yoda-1.9.7-3 = x
+++ test x/sw/slc7_x86-64/fastjet/v3.4.1_1.052-alice2-4 = x
+++ test x/sw/slc7_x86-64/cgal/4.6.3-129 = x
+++ test x/sw/slc7_x86-64/GMP/v6.2.1-43 = x

The script snippet rivet_3rdparty.sh checks if all variables are set, as they evidently are - the last 7 lines starting with test - and if not, exists the current shell. This seems to happen for no apparent reason, and without a message - very strange - I think it is likely some weird stuff with source. New commit will hopefully fix this - I do it differently by setting a variable ret=true or ret=false in case of errors, and then execute that variable at the end of the snippet.

@cholmcc
Copy link
Copy Markdown
Contributor Author

cholmcc commented Nov 30, 2023

Hi all,

This last commit fixed the build issue. The question is if this will also fix the run-time issue 😄 To settle that, I guess we need a merge and deployment on LXPLUS. Please let me know when I can try that out. Thanks.

Yours,
Christian

@cholmcc
Copy link
Copy Markdown
Contributor Author

cholmcc commented Dec 1, 2023

build/O2/alidist-dataflow-cs8 and build/O2/alidist-ubuntu2204 fails due to unrelated issue in CCDB test,

@maireiphc maireiphc merged commit 8d5c726 into alisw:master Dec 3, 2023
@maireiphc
Copy link
Copy Markdown
Contributor

Thanks Christian for you detail answer (subtle tricks around...) and further investigations.
Ok, let's give a shot with deployment via CVMFS.
To be monitored.

@cholmcc
Copy link
Copy Markdown
Contributor Author

cholmcc commented Dec 4, 2023

Hi all,

Doing rivet-build ALICE_YYYY_I1234567.cc worked! So far so good. However, I still see paths like

/local/workspace/DailyBuilds/DailyAliGenerators/daily-tags.RM4czFHN18/slc7_x86-64/cgal/4.6.3-129/lib
/local/workspace/DailyBuilds/DailyAliGenerators/daily-tags.jh7bRB3Qc7/slc7_x86-64/GMP/v6.2.1-43/lib 

in rivet-build and rivet-config. Not a major show-stopper, but not the intended outcome. Will take a closer look, and get back to you.

Yours,

Christian

@cholmcc
Copy link
Copy Markdown
Contributor Author

cholmcc commented Dec 20, 2023

Hi all,

I've been away for a week or so, and I'm not quite sure were we are. Could someone remind me? Thanks.

Also, as I said above, there's still some stuff left in the scripts when build on AliDist. Could someone remind me how to use docker to test builds? I think I would need a bit closer look at what is going on. Thanks.

Also, Merry Christmas 😄

Yours,
Christian

@cholmcc cholmcc deleted the patch-3 branch January 18, 2024 00:49
MichaelLettrich pushed a commit to MichaelLettrich/alidist that referenced this pull request Oct 3, 2024
…isw#5264)

* Update rivet.sh - set 3rd party paths via script, not `_ROOT` env

See alisw#5245

* Update rivet.sh - fixed wrong name

Whoops, `grep`'ed  for `cgal` where I wanted to grep for `GMP`.

* Update rivet.sh - deduce variables at build-time as well as at run-time

Following discussion on this MR, I made some changes so that at build-time, we deduce variables in same way as done at run-time (uses the same script snippet). 

The script snippet has been made more robust, and will exit out (build, rivet-config, rivet-build) in case of problems. 
If variables are set before hand (e.g., `HEPMC3_ROOT` is already set), then that value is used as-is.

* Update rivet.sh - try to fix error in rivet_3rdparty.sh
See 'env_err' test variables with 'ret=true', likely needed to make the 'source rivet_3rdparty.sh' command fly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants