[PATCH] gnu: maths: petsc: Reduce closure size.

  • Done
  • quality assurance status badge
Details
3 participants
  • Efraim Flashner
  • Lars Bilke
  • Ludovic Courtès
Owner
unassigned
Submitted by
Lars Bilke
Severity
normal

Debbugs page

Lars Bilke wrote 1 years ago
(address . guix-patches@gnu.org)(name . Lars Bilke)(address . lars.bilke@ufz.de)
9ba967c61c66acea4b4c3b1db6e1a446ad9e827e.1702558568.git.lars.bilke@ufz.de
Reduces closure size by around 350 MB.

May break CMake-based projects which use the following script:


Use pkg-config based finding instead. See

Change-Id: I2e6900747b2118546f0a39ceb109b3f2f90e6949
---
gnu/packages/maths.scm | 2 ++
1 file changed, 2 insertions(+)

Toggle diff (17 lines)
diff --git a/gnu/packages/maths.scm b/gnu/packages/maths.scm
index 0f4d29b40f..7f3e80efa4 100644
--- a/gnu/packages/maths.scm
+++ b/gnu/packages/maths.scm
@@ -3484,6 +3484,8 @@ (define-public petsc
'("configure.log" "make.log" "gmake.log"
"test.log" "error.log" "RDict.db"
"PETScBuildInternal.cmake"
+ "petscvariables"
+ "configure-hash"
;; Once installed, should uninstall with Guix
"uninstall.py")))))
(add-after 'install 'move-examples

base-commit: ac61e9705fb8c450c6cd0c1731fbb1b909c1f944
--
2.43.0
Lars Bilke wrote 1 years ago
[PATCH v2] gnu: maths: petsc: Reduce closure size.
(address . 67822@debbugs.gnu.org)(name . Lars Bilke)(address . lars.bilke@ufz.de)
5b3a72695c82aa1c46e79ed30004d30d56df61ea.1702565587.git.lars.bilke@ufz.de
Reduces closure size by around 350 MB by removing refernces to build
dependencies (e.g. gcc).

The v2 patch does not delete petscvariables but only removes some store
references. This fixes downstream packages such as e.g. dealii-openmpi.

Change-Id: I2e6900747b2118546f0a39ceb109b3f2f90e6949
---
gnu/packages/maths.scm | 12 ++++++++++++
1 file changed, 12 insertions(+)

Toggle diff (29 lines)
diff --git a/gnu/packages/maths.scm b/gnu/packages/maths.scm
index 0f4d29b40f..8e8e380336 100644
--- a/gnu/packages/maths.scm
+++ b/gnu/packages/maths.scm
@@ -3484,8 +3484,20 @@ (define-public petsc
'("configure.log" "make.log" "gmake.log"
"test.log" "error.log" "RDict.db"
"PETScBuildInternal.cmake"
+ "configure-hash"
;; Once installed, should uninstall with Guix
"uninstall.py")))))
+ (add-after 'clean-install 'clear-reference-to-compiler
+ (lambda* (#:key inputs outputs #:allow-other-keys)
+ ;; Do not retain a reference to GCC and other build only inputs.
+ (let ((out (assoc-ref outputs "out")))
+ (substitute* (string-append out "/lib/petsc/conf/petscvariables")
+ (("/gnu/store/.*/bin/gcc") "gcc")
+ (("/gnu/store/.*/bin/g++") "g++")
+ (("/gnu/store/.*/bin/make") "make")
+ (("/gnu/store/.*/bin/diff") "diff")
+ (("/gnu/store/.*/bin/sed") "sed")
+ ))))
(add-after 'install 'move-examples
(lambda* (#:key outputs #:allow-other-keys)
(let* ((out (assoc-ref outputs "out"))

base-commit: ac61e9705fb8c450c6cd0c1731fbb1b909c1f944
--
2.43.0
Lars Bilke wrote 1 years ago
[PATCH v3] gnu: maths: petsc: Reduce closure size.
(address . 67822@debbugs.gnu.org)(name . Lars Bilke)(address . lars.bilke@ufz.de)
6a6413853ce16af65e143bd727715be6dd7d6048.1702630557.git.lars.bilke@ufz.de
Reduces closure size by around 350 MB by removing refernces to build dependencies (e.g. gcc).

Fixed a regex from v2.

Change-Id: I2e6900747b2118546f0a39ceb109b3f2f90e6949
---
gnu/packages/maths.scm | 12 ++++++++++++
1 file changed, 12 insertions(+)

Toggle diff (29 lines)
diff --git a/gnu/packages/maths.scm b/gnu/packages/maths.scm
index 0f4d29b40f..4b2643e535 100644
--- a/gnu/packages/maths.scm
+++ b/gnu/packages/maths.scm
@@ -3484,8 +3484,20 @@ (define-public petsc
'("configure.log" "make.log" "gmake.log"
"test.log" "error.log" "RDict.db"
"PETScBuildInternal.cmake"
+ "configure-hash"
;; Once installed, should uninstall with Guix
"uninstall.py")))))
+ (add-after 'clean-install 'clear-reference-to-compiler
+ (lambda* (#:key inputs outputs #:allow-other-keys)
+ ;; Do not retain a reference to GCC and other build only inputs.
+ (let ((out (assoc-ref outputs "out")))
+ (substitute* (string-append out "/lib/petsc/conf/petscvariables")
+ (("/gnu/store/.*/bin/gcc") "gcc")
+ (("/gnu/store/.*/bin/g\\+\\+") "g++")
+ (("/gnu/store/.*/bin/make") "make")
+ (("/gnu/store/.*/bin/diff") "diff")
+ (("/gnu/store/.*/bin/sed") "sed")
+ ))))
(add-after 'install 'move-examples
(lambda* (#:key outputs #:allow-other-keys)
(let* ((out (assoc-ref outputs "out"))

base-commit: ac61e9705fb8c450c6cd0c1731fbb1b909c1f944
--
2.43.0
Ludovic Courtès wrote 1 years ago
(name . Lars Bilke)(address . lars.bilke@ufz.de)
878r54f35z.fsf@gnu.org
Hi Lars,

Lars Bilke <lars.bilke@ufz.de> skribis:

Toggle quote (2 lines)
> Reduces closure size by around 350 MB by removing refernces to build dependencies (e.g. gcc).

Woow, nice!

Toggle quote (11 lines)
> + (add-after 'clean-install 'clear-reference-to-compiler
> + (lambda* (#:key inputs outputs #:allow-other-keys)
> + ;; Do not retain a reference to GCC and other build only inputs.
> + (let ((out (assoc-ref outputs "out")))
> + (substitute* (string-append out "/lib/petsc/conf/petscvariables")
> + (("/gnu/store/.*/bin/gcc") "gcc")
> + (("/gnu/store/.*/bin/g\\+\\+") "g++")
> + (("/gnu/store/.*/bin/make") "make")
> + (("/gnu/store/.*/bin/diff") "diff")
> + (("/gnu/store/.*/bin/sed") "sed")

Can we instead patch the thing that creates ‘petscvariables’ in the
first place?

The reason I’m suggesting it is because in general we avoid hardcoding
/gnu/store in substitution patterns because it’s possible to configure
Guix with a different store directory.

Thanks, and apologies for the delay!

Ludo’.
Lars Bilke wrote 1 years ago
(name . Ludovic Courtès)(address . ludo@gnu.org)
68C18595-278B-4AB1-9260-B154E7BC8E60@ufz.de
Hi Ludo,

On 5 Jan 2024, at 12:08, Ludovic Courtès wrote:

Toggle quote (7 lines)
> Can we instead patch the thing that creates ‘petscvariables’ in the
> first place?
>
> The reason I’m suggesting it is because in general we avoid hardcoding
> /gnu/store in substitution patterns because it’s possible to configure
> Guix with a different store directory.

Thanks for your feedback!

In v1 of this patch I removed the 'petscvariables'-file completely but this broke dependent packages as well as not-yet packaged projects which use the file for finding the PETSc library and configuriung their build system.

Is there a possibility to replace the hard-coded /gnu/store with a variable which evaluates to the current store directory?

Sincerely,
Lars
Attachment: smime.p7s
Efraim Flashner wrote 1 years ago
(name . Lars Bilke)(address . lars.bilke@ufz.de)
ZZpqX6OYjXu-lfB9@3900XT
On Fri, Jan 05, 2024 at 12:52:02PM +0100, Lars Bilke wrote:
Toggle quote (17 lines)
> Hi Ludo,
>
> On 5 Jan 2024, at 12:08, Ludovic Courtès wrote:
>
> > Can we instead patch the thing that creates ‘petscvariables’ in the
> > first place?
> >
> > The reason I’m suggesting it is because in general we avoid hardcoding
> > /gnu/store in substitution patterns because it’s possible to configure
> > Guix with a different store directory.
>
> Thanks for your feedback!
>
> In v1 of this patch I removed the 'petscvariables'-file completely but this broke dependent packages as well as not-yet packaged projects which use the file for finding the PETSc library and configuriung their build system.
>
> Is there a possibility to replace the hard-coded /gnu/store with a variable which evaluates to the current store directory?

There's %store-directory in (guix build utils). In fact, it looks like
git might have some code that you can borrow.


--
Efraim Flashner <efraim@flashner.co.il> רנשלפ םירפא
GPG key = A28B F40C 3E55 1372 662D 14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted
-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEoov0DD5VE3JmLRT3Qarn3Mo9g1EFAmWaal8ACgkQQarn3Mo9
g1H3zQ/+PXd0WuLcYIxyFs9CP64itsYta23aqEjpMiY3fj/uXwvgU4DA92LEwi0a
+KxtH7+UcKrL4bjiTp3j4CEQXT4f0pYsBuhB/Y41WN5KVy+oh8GZ4ldEW0JAEu/y
t1WsmiYQZYILHDYgWoT9KlcoRWHkc0YPPm80+xnSIuLB2LHCTyqO9xhnUzAKNgXK
3u38I2alt1z9ZVECs1NKbimsvSucHvNbdEf6XGpXQlkD2ZzkLQhP2UPQFR70CHpn
2kO9HTDV3TsWHHG8AJWlQKtDeOCVpZA5B6AnejiU3AS2s672aix222oRIKsIxNjz
GanCeaAnrIb+eVqcuOyn+dBL69tXHHewnlULqJtEzBWDS/5KZDfYIaIEi2vu+Rvt
ceggNLNF/CUPqIZDD1kGUesOg+oa8vBX9JugB/2llQvyV7NH173cICDOuMNMRM/w
xgq3wrT1/ePNLf3WUq7NMJQWDZEV+EocskF5UfP/KlCFtcOpWWiazF0mXVla3/eU
UOGGpmC1LjEJCXXvtbWqFFP7zNkKfFcMnWXKVL5Kxn5jGjCQTlRUVdY922IoqoLe
LY0apigkKwGtA4V0QNo11Jy9iha6w2pjVSytMUMeDsG9uTLbBAbS2iCeEkkjxomd
nvMyOhmm0uROGPIAO/GgRmrbl4jcVrvGQ0EuN9CpvODnIrDBs9I=
=ssFl
-----END PGP SIGNATURE-----


Ludovic Courtès wrote 1 years ago
(name . Efraim Flashner)(address . efraim@flashner.co.il)
87zfxf3fp4.fsf@gnu.org
Hi,

Efraim Flashner <efraim@flashner.co.il> skribis:

Toggle quote (21 lines)
> On Fri, Jan 05, 2024 at 12:52:02PM +0100, Lars Bilke wrote:
>> Hi Ludo,
>>
>> On 5 Jan 2024, at 12:08, Ludovic Courtès wrote:
>>
>> > Can we instead patch the thing that creates ‘petscvariables’ in the
>> > first place?
>> >
>> > The reason I’m suggesting it is because in general we avoid hardcoding
>> > /gnu/store in substitution patterns because it’s possible to configure
>> > Guix with a different store directory.
>>
>> Thanks for your feedback!
>>
>> In v1 of this patch I removed the 'petscvariables'-file completely but this broke dependent packages as well as not-yet packaged projects which use the file for finding the PETSc library and configuriung their build system.
>>
>> Is there a possibility to replace the hard-coded /gnu/store with a variable which evaluates to the current store directory?
>
> There's %store-directory in (guix build utils). In fact, it looks like
> git might have some code that you can borrow.

Yes. However, I think we should use literal strings for patterns in
‘substitute*’. That is, I would avoid:

(substitute* …
(((string-append (%store-directory) "/bin/whatever"))
…))

in favor of, say:

(substitute* …
(("([[:graph:]]+)/bin/whatever")
…))

This is to make things easier to understand, 100% correct (in theory we
should use ‘regexp-quote’ when turning strings into regexps), and to
leave room for how ‘substitute*’ is implemented.

Thanks,
Ludo’.
Lars Bilke wrote 1 years ago
[PATCH v4] gnu: maths: petsc: Reduce closure size.
(address . 67822@debbugs.gnu.org)(name . Lars Bilke)(address . lars.bilke@ufz.de)
a2c44dd68bd3b7ce4ebad81cf22c667caebc586e.1704824104.git.lars.bilke@ufz.de
Reduces closure size by around 350 MB by removing refernces to build
dependencies (e.g. gcc).

New ion v4:

Used proposed :graph: syntax.
Also removed gfortran reference.

Change-Id: I2e6900747b2118546f0a39ceb109b3f2f90e6949
---
gnu/packages/maths.scm | 13 +++++++++++++
1 file changed, 13 insertions(+)

Toggle diff (30 lines)
diff --git a/gnu/packages/maths.scm b/gnu/packages/maths.scm
index adc7beb655..7dc93ce8ee 100644
--- a/gnu/packages/maths.scm
+++ b/gnu/packages/maths.scm
@@ -3484,8 +3484,21 @@ (define-public petsc
'("configure.log" "make.log" "gmake.log"
"test.log" "error.log" "RDict.db"
"PETScBuildInternal.cmake"
+ "configure-hash"
;; Once installed, should uninstall with Guix
"uninstall.py")))))
+ (add-after 'clean-install 'clear-reference-to-compiler
+ (lambda* (#:key inputs outputs #:allow-other-keys)
+ ;; Do not retain a reference to GCC and other build only inputs.
+ (let ((out (assoc-ref outputs "out")))
+ (substitute* (string-append out "/lib/petsc/conf/petscvariables")
+ (("([[:graph:]]+)/bin/gcc") "gcc")
+ (("([[:graph:]]+)/bin/g\\+\\+") "g++")
+ (("([[:graph:]]+)/bin/make") "make")
+ (("([[:graph:]]+)/bin/diff") "diff")
+ (("([[:graph:]]+)/bin/sed") "sed")
+ (("([[:graph:]]+)/bin/gfortran") "gfortran")
+ ))))
(add-after 'install 'move-examples
(lambda* (#:key outputs #:allow-other-keys)
(let* ((out (assoc-ref outputs "out"))

base-commit: a1a645337a76cf5fb55d20ee694fcb6a52fcf11b
--
2.43.0
Lars Bilke wrote 1 years ago
(address . ludo@gnu.org)(address . 67822@debbugs.gnu.org)
77AE81C2-3C38-4FBC-AA26-E8C1759F480B@ufz.de
Dear Ludo,

may I kindly ask for another review here? I have implemented the suggestions in v4 and would like to benefit from the size reduction as I am preparing some container-based computations on several HPC clusters which do not have Guix installed ( yet ;-) ).

Thanks a lot!
Lars

On 9 Jan 2024, at 19:15, Lars Bilke wrote:

Toggle quote (43 lines)
> Reduces closure size by around 350 MB by removing refernces to build
> dependencies (e.g. gcc).
>
> New ion v4:
>
> Used proposed :graph: syntax.
> Also removed gfortran reference.
>
> Change-Id: I2e6900747b2118546f0a39ceb109b3f2f90e6949
> ---
> gnu/packages/maths.scm | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/gnu/packages/maths.scm b/gnu/packages/maths.scm
> index adc7beb655..7dc93ce8ee 100644
> --- a/gnu/packages/maths.scm
> +++ b/gnu/packages/maths.scm
> @@ -3484,8 +3484,21 @@ (define-public petsc
> '("configure.log" "make.log" "gmake.log"
> "test.log" "error.log" "RDict.db"
> "PETScBuildInternal.cmake"
> + "configure-hash"
> ;; Once installed, should uninstall with Guix
> "uninstall.py")))))
> + (add-after 'clean-install 'clear-reference-to-compiler
> + (lambda* (#:key inputs outputs #:allow-other-keys)
> + ;; Do not retain a reference to GCC and other build only inputs.
> + (let ((out (assoc-ref outputs "out")))
> + (substitute* (string-append out "/lib/petsc/conf/petscvariables")
> + (("([[:graph:]]+)/bin/gcc") "gcc")
> + (("([[:graph:]]+)/bin/g\\+\\+") "g++")
> + (("([[:graph:]]+)/bin/make") "make")
> + (("([[:graph:]]+)/bin/diff") "diff")
> + (("([[:graph:]]+)/bin/sed") "sed")
> + (("([[:graph:]]+)/bin/gfortran") "gfortran")
> + ))))
> (add-after 'install 'move-examples
> (lambda* (#:key outputs #:allow-other-keys)
> (let* ((out (assoc-ref outputs "out"))
>
> base-commit: a1a645337a76cf5fb55d20ee694fcb6a52fcf11b
> --
> 2.43.0
Attachment: smime.p7s
Ludovic Courtès wrote 1 years ago
Re: [bug#67822] [PATCH v4] gnu: maths: petsc: Reduce closure size.
(name . Lars Bilke)(address . lars.bilke@ufz.de)
87wmqqi6c6.fsf@gnu.org
Hi,

Lars Bilke <lars.bilke@ufz.de> skribis:

Toggle quote (10 lines)
> Reduces closure size by around 350 MB by removing refernces to build
> dependencies (e.g. gcc).
>
> New ion v4:
>
> Used proposed :graph: syntax.
> Also removed gfortran reference.
>
> Change-Id: I2e6900747b2118546f0a39ceb109b3f2f90e6949

[...]

Toggle quote (4 lines)
> + (("([[:graph:]]+)/bin/sed") "sed")
> + (("([[:graph:]]+)/bin/gfortran") "gfortran")
> + ))))

I move those lonely parens to the previous line, tweaked the commit log,
and applied it. Thank you!

Ludo’.
Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 67822@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 67822
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch
You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags
mumi command -t +confirmed -t +easy
Or, remove the moreinfo tag and set the help tag
mumi command -t -moreinfo -t +help