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.

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

Fix for problems in rivet-config, rivet-build and fastjet-config #5320

Merged
merged 7 commits into from
Jan 26, 2024

Conversation

cholmcc
Copy link
Contributor

@cholmcc cholmcc commented Jan 18, 2024

...rivet-config, rivet-build, and - I suspect - fastjet-config.

See also #5245 for a discussion.

Note, the rivet.sh script is set up to explicitly fail - thus hopefully allowing me to see the log output.

This is work-in-progress.

…rivet-config`, `rivet-build`, and - I suspect - `fastjet-config`.

See also alisw#5245 for a discussion.

Note, the `rivet.sh` script is set up to explicitly fail - thus hopefully allowing me to see the log output.

This is work-in-progress.
@cholmcc cholmcc requested review from a team as code owners January 18, 2024 12:35
@cholmcc cholmcc marked this pull request as draft January 18, 2024 12:35
jackal1-66
jackal1-66 previously approved these changes Jan 18, 2024
Copy link
Contributor

@jackal1-66 jackal1-66 left a comment

Choose a reason for hiding this comment

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

Approving the changes to fix Rivet deployment

@cholmcc
Copy link
Contributor Author

cholmcc commented Jan 18, 2024

@jackal1-66 and @ktf Please see this test MR for more on figuring out what's up with the Rivet deployment.

This is really a test MR to see if I can deduce from the build logs whats going on. I hope we will get the build logs. If not, is there some other way we can get at them? Thanks.

@cholmcc
Copy link
Contributor Author

cholmcc commented Jan 18, 2024

Approving the changes to fix Rivet deployment

Whoops, a little early I think. The rivet.sh script is designed to fail (with exit 1). This PR is really only to trigger a build and get to the logs.

@jackal1-66
Copy link
Contributor

@cholmcc I thought some tests couldn't start without an approved review (as for other packages), Of course I didn't approve the full merge ;)

@cholmcc
Copy link
Contributor Author

cholmcc commented Jan 18, 2024

@cholmcc I thought some tests couldn't start without an approved review (as for other packages), Of course I didn't approve the full merge ;)

OK, I see - thanks.

@cholmcc
Copy link
Contributor Author

cholmcc commented Jan 18, 2024

Maybe it's the WIP in the title - I'll remove that 😄

@cholmcc cholmcc changed the title WIP: This is a test to see if I can figure out where the problem is with `… This is a test to see if I can figure out where the problem is with `… Jan 18, 2024
@cholmcc
Copy link
Contributor Author

cholmcc commented Jan 18, 2024

Still not running - will try to do "Ready for review" - should still not be merged

@cholmcc cholmcc marked this pull request as ready for review January 18, 2024 23:32
The combination of special handling for GMP and CGal seems to do the
trick, together with the update to the fastjet build script.

Note, we may not need the special handling anymore, but I leave it in in
case we are building against an older fastjet distribution.
@cholmcc
Copy link
Contributor Author

cholmcc commented Jan 19, 2024

Hi,

I pushed a new commit. This time I believe it fixed the issue - at least from what I saw in the last log.

It seems that the special handling of GMP and CGal, together with the updates to the fastjet-config.sh script, did the trick. Note that the special handling may not be needed if we're building against updated fastjet, but I leave it in in case we are building against an older fastjet

If this passes the CI tests, I think it is ready for deployment.

Thanks.

@cholmcc
Copy link
Contributor Author

cholmcc commented Jan 19, 2024

build/O2/fullCI/alidist seems to fail due to timeout - unrelated to this MR.

@cholmcc
Copy link
Contributor Author

cholmcc commented Jan 19, 2024

Even though the build/O2/fullCI/alidist test failed, I think this PR is ready to be merged and deployed. Maybe you want to trigger a new round of test to see if the build/O2/fullCI/alidist will succeed.

Copy link
Contributor

@jackal1-66 jackal1-66 left a comment

Choose a reason for hiding this comment

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

The code looks good to me, just a small comment ont he last EOF of the rivet.sh script which seems not to serve a purpose

rivet.sh Show resolved Hide resolved
jackal1-66
jackal1-66 previously approved these changes Jan 22, 2024
@jackal1-66 jackal1-66 enabled auto-merge (squash) January 22, 2024 15:34
@jackal1-66
Copy link
Contributor

@ktf could you please review the changes? Your approval is required to fix this issue.

@cholmcc cholmcc changed the title This is a test to see if I can figure out where the problem is with `… Fix for problems in rivet-config, rivet-build and fastjet-config Jan 23, 2024
- fastjet module need to load GMP to make sure we use the right version
- Fix a typo in generated environment script for Rivet
auto-merge was automatically disabled January 23, 2024 15:36

Head branch was pushed to by a user without write access

jackal1-66
jackal1-66 previously approved these changes Jan 24, 2024
@cholmcc
Copy link
Contributor Author

cholmcc commented Jan 26, 2024

Neither failure of build/O2/alidist-ubuntu2204 nor build/O2/fullCI/alidst are related to this PR. The first is a failure in the test-suite of $O^2$ (Utilities/Mergers/test/test_Algorithm.cxx) and the latter is caused by a time-out. Perhaps the CI's should be re-run, or the merge force through anyways. Thanks.

@ktf ktf disabled auto-merge January 26, 2024 08:17
@ktf ktf merged commit 390b9a6 into alisw:master Jan 26, 2024
10 of 12 checks passed
@cholmcc
Copy link
Contributor Author

cholmcc commented Jan 26, 2024

lxplus:~> alienv enter Rivet/rivet-3.1.8-12 
[Rivet/rivet-3.1.8-12] ~ > ls -l /cvmfs/alice.cern.ch/el7-x86_64/Packages/Rivet/rivet-3.1.8-12/bin/rivet-config 
-rwxr-xr-x. 1 cvmfs cvmfs 4369 Jan 26 18:08 /cvmfs/alice.cern.ch/el7-x86_64/Packages/Rivet/rivet-3.1.8-12/bin/rivet-config
[Rivet/rivet-3.1.8-12] ~ > rivet-config --libs | tr ' ' '\n  '
-L/cvmfs/alice.cern.ch/el7-x86_64/Packages/Rivet/rivet-3.1.8-12/lib
-L/cvmfs/alice.cern.ch/el7-x86_64/Packages/HepMC3/3.2.5-111/lib
-lHepMC3
-lHepMC3search
-L/cvmfs/alice.cern.ch/el7-x86_64/Packages/YODA/yoda-1.9.7-8/lib
-lYODA
-L/cvmfs/alice.cern.ch/el7-x86_64/Packages/fastjet/v3.4.1_1.052-alice2-10/lib
-lfastjettools
-lfastjet
-lCGAL
-lgmp
-L/cvmfs/alice.cern.ch/el7-x86_64/Packages/cgal/4.6.3-131/cgal/4.6.3-131/lib
-L/cvmfs/alice.cern.ch/el7-x86_64/Packages/GMP/v6.2.1-43/lib
-Wl,-rpath,/cvmfs/alice.cern.ch/el7-x86_64/Packages/GMP/v6.2.1-43/lib
-lm
-lfastjetplugins
-lsiscone_spherical
-lsiscone
-lfastjetcontribfragile
-lfastjettools
-lRivet

Got it 😄 - well almost. The cgal path got an extra cgal/4.6.3-131/lib on it, which breaks rivet-build - OK, I check it again an make a new MR.

However

[Rivet/rivet-3.1.8-12] ~ > ls -l `which fastjet-config` 
-rwxr-xr-x. 1 cvmfs cvmfs 13775 Jan 26 18:08 /cvmfs/alice.cern.ch/el7-x86_64/Packages/fastjet/v3.4.1_1.052-alice2-10/bin/fastjet-config
[Rivet/rivet-3.1.8-12] ~ > fastjet-config --libs | tr ' ' '\n' 
-Wl,-rpath,/cvmfs/alice.cern.ch/el7-x86_64/Packages/fastjet/v3.4.1_1.052-alice2-10/lib
-L/cvmfs/alice.cern.ch/el7-x86_64/Packages/fastjet/v3.4.1_1.052-alice2-10/lib
-lfastjettools
-lfastjet
-lCGAL
-lgmp
-L/cvmfs/alice.cern.ch/el7-x86_64/Packages/cgal/4.6.3-131/lib
-Wl,-rpath,/cvmfs/alice.cern.ch/el7-x86_64/Packages/cgal/4.6.3-131/lib
-L/local/workspace/DailyBuilds/DailyAliGenerators/daily-tags.5pNdhyUFdk/slc7_x86-64/GMP/v6.2.1-43/lib
-Wl,-rpath,/local/workspace/DailyBuilds/DailyAliGenerators/daily-tags.5pNdhyUFdk/slc7_x86-64/GMP/v6.2.1-43/lib
-lm

so the fastjet-config script reports the build-time path of GMP, still (the cgal one got fixed, though). It is not a show stopper, because the hacking in rivet.sh recipe fixes the issue. But it would be nice if fastjet-config also did the right thing. I will see if I can fix this too, but as it's not a show stopper, I think I won't spend too much time on it.

Yours,
Christian

@cholmcc
Copy link
Contributor Author

cholmcc commented Jan 26, 2024

BTW, just doing

alienv enter Rivet 
[Rivet] ~ > which rivet-config
/cvmfs/alice.cern.ch/el7-x86_64/Packages/Rivet/rivet-3.1.8-9/bin/rivet-config

doesn't select the newest install (rivet-3.1.8-12) but rather rivet-3.1.8-9. Is it based on a lexical sort?

Yours,
Christian

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.

None yet

3 participants