Subject: Re: [bug#60266] [PATCH] gnu: Add form.

From: Antero Mejr <[email-masked]>
Date: Thu, 16 Mar 2023 01:15:51 +0000

Eric Bavier <[email-masked]> writes:

>> x86_64 only due to test failures on other platforms. Developers
>> say other platforms are not "tier 1" supported:
>> https://github.com/vermaseren/form/issues/426
>
> This may be better as a comment near the `supported-systems` field,
> along with a short summary of which tests fail on other systems.

Added comments in v2.

> This commit is downstream from the 4.3.0 release, so we should include
> in a comment the rationale for not using just the 4.3.0 release
> tarball.

Added rationale.

>> +                (snippet #~(substitute* "check/examples.frm"
>> +                             (("#pend_if valgrind\\?")
>> +                              "#pend_if 0")))))
>
> This snippet appears to be related to the specifics of this package
> build?  E.g. if someone were to grab the source with `guix build -S
> form`, they would not be able to have valgrind support, correct?  If
> that's the case, perhaps it would be better to apply this substitution
> in a build phase.

No, it's disabling a failing test that has a known problem (memory
leak). Added a comment with the explanation.

> For some other maths packages that have both a serial and MPI versions,
> we've usually provided two packages.  See e.g. `petsc` and `petsc-
> openmpi`.  This can be useful if someone does not need a full MPI-
> capable version of form, and the separation is easily achieved.

Moved the openMPI version to the parform package.

> Also, I see the default compilation flags include `-march=native`,
> which will most likely cause a problem with build farm substitutes or
> `guix challenge`.  Could you figure out have to override these flags?
> Guix usually assumes `SSE2` capabilities for x86_64 targets, iirc. 
> Bonus points for enabling a "tunable" package (c.f. "Package
> Transformation Options").

Disabled --march=native in v2.

> Maybe we should use https://www.nikhef.nl/~form/ instead? This is the
> URL specified in the included manpage.

I used the other homepage so people can skip the weird "license
agreement" page at https://www.nikhef.nl/~form/. But I changed it to
your suggestion in v2.

Thanks for the review.