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

update custom easyblock for PETSc to consider include/suitesparse subdirectory for SuiteSparse headers #3391

Merged
merged 3 commits into from
Sep 4, 2024

Conversation

tanmoy1989
Copy link
Contributor

@tanmoy1989 tanmoy1989 commented Jul 10, 2024

(created using eb --new-pr)
SuiteSparse can install its headers into a subdirectory of the include directory instead. To be safe, both flavours of include directory locations were considered for SuiteSparse.

@tanmoy1989 tanmoy1989 changed the title inclding subdirectory of include for SuiteSparse Incorporating subdirectory path of include for SuiteSparse Jul 10, 2024
@zao
Copy link
Contributor

zao commented Jul 16, 2024

Test report by @zao

Overview of tested easyconfigs (in order)

Build succeeded for 0 out of 1 (1 easyconfigs in total)
eb-mix.zao.se - Linux Rocky Linux 9.4 (Blue Onyx), x86_64, AMD Ryzen 9 3900X 12-Core Processor (zen2), Python 3.9.18
See https://gist.github.com/zao/4fa75f6450d7a51a0a2a1c51522b2df3 for a full test report.

@zao
Copy link
Contributor

zao commented Jul 16, 2024

It seems like the PETSc build gets angry if any of the SuiteSparse include directory candidates given on the command line doesn't exist, see the test report in #3391 (comment) above.

This logic probably needs to guard the additional entry behind whether the directory actually exists on the filesystem.

@tanmoy1989
Copy link
Contributor Author

Thanks @zao: I would really appreciate if you could provide suggestions on how to proceed further. I am running out of options and a bit confused too.

Co-authored-by: ocaisa <alan.ocais@cecam.org>
easybuild/easyblocks/p/petsc.py Outdated Show resolved Hide resolved
@boegel boegel added the bug fix label Jul 31, 2024
@boegel boegel added this to the 4.x milestone Jul 31, 2024
@boegel boegel changed the title Incorporating subdirectory path of include for SuiteSparse update custom easyblock for PETSc to consider include/suitesparse subdirectory for SuiteSparse headers Jul 31, 2024
@boegel
Copy link
Member

boegel commented Jul 31, 2024

@tanmoy1989 While I'm not opposing the proposed change, as long as it doesn't cause trouble with builds that were working fine before, I do wonder what's triggering the different subdirectory to be used instead?

@boegel boegel changed the title update custom easyblock for PETSc to consider include/suitesparse subdirectory for SuiteSparse headers update custom easyblock for PETSc to consider include/suitesparse subdirectory for SuiteSparse headers Jul 31, 2024
@tanmoy1989
Copy link
Contributor Author

@boegel: I am not sure if I understood your question completely, however, I am fully Ok if you suggest any changes to be made before merging the PR.

@boegel
Copy link
Member

boegel commented Aug 13, 2024

@tanmoy1989 I guess my question is what triggered you to look into implemented these changes.

I'm not sure how come we did not run into this before, but maybe I'm missing something...

@tanmoy1989
Copy link
Contributor Author

tanmoy1989 commented Aug 14, 2024

@boegel Well, the main triggering point was the stage where PETSc was failing to build. Then @zao suggested some changes (here and over EasyBuild slack) and I followed that which ultimately brings down to this custom easyblock. The .eb file for PETSc can be found here where a detailed discussion with @zao took place.

@boegel
Copy link
Member

boegel commented Aug 28, 2024

@tanmoy1989 Thanks for clarifying, I'll see if I can get this merged ASAP to be included in the upcoming EasyBuild v4.9.3...

There's a small code style issue to fix:

./easybuild/easyblocks/p/petsc.py:269:23: E111 indentation is not a multiple of 4

We can deal with this ourselves though if that's easier for you

@tanmoy1989
Copy link
Contributor Author

@boegel Many thanks! I would really appreciate if the indentation part could be kindly taken care of.

easybuild/easyblocks/p/petsc.py Outdated Show resolved Hide resolved
@boegel boegel modified the milestones: 4.x, release after 4.9.2 Aug 28, 2024
Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@boegel
Copy link
Member

boegel commented Sep 4, 2024

@boegelbot please test @ generoso
EB_ARGS="PETSc-3.19.2-foss-2022b.eb PETSc-3.20.3-foss-2023a.eb --installpath /tmp/$USER/pr3391"

@boegelbot
Copy link

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

PR test command 'EB_PR=3391 EB_ARGS="PETSc-3.19.2-foss-2022b.eb PETSc-3.20.3-foss-2023a.eb --installpath /tmp/$USER/pr3391" EB_CONTAINER= EB_REPO=easybuild-easyblocks /opt/software/slurm/bin/sbatch --job-name test_PR_3391 --ntasks=4 ~/boegelbot/eb_from_pr_upload_generoso.sh' executed!

  • exit code: 0
  • output:
Submitted batch job 14185

Test results coming soon (I hope)...

- notification for comment with ID 2329225545 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).

@boegelbot
Copy link

Test report by @boegelbot

Overview of tested easyconfigs (in order)

  • SUCCESS PETSc-3.19.2-foss-2022b.eb
  • SUCCESS PETSc-3.20.3-foss-2023a.eb

Build succeeded for 2 out of 2 (2 easyconfigs in total)
cns3 - 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/20ed88bcf6dfc9f9ef015172e3380c8f for a full test report.

@boegel boegel merged commit cdc4930 into easybuilders:develop Sep 4, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants