[bug#45573] [PATCH] Correct freecad runtime errors

[Leo Famulari]

On Thu, Dec 31, 2020 at 06:47:59PM +0000, Ekaitz Zarraga wrote:
> I attach 4 patches that correct the runtime issues freecad has and also 
> supply some packages needed as a dependency.
> 
> Freecad is a very complex package that is hard to build because many of its 
> dependencies' buildsystem is broken. The approach followed here is the same 
> that Nix follows and appears to work correctly.
> 
> The changes correct the Draft module, which wasn't available because pivy was 
> not added as a dependency.
> 
> For context, see this message:
> https://lists.gnu.org/archive/html/guix-devel/2020-12/msg00123.html

Thanks!

Here is my feedback:

> Subject: [PATCH 1/4] gnu: Add coin3D-4.
> 
>     * gnu/packages/graphics.scm (coin3D-4): New variable.

> +        (snippet
> +          '(begin
> +             (for-each delete-file
> +                       '("cfg/csubst.exe"
> +                         "cfg/wrapmsvc.exe"))

Please add a brief code comment like "Delete binaries".

> +             (substitute* "CMakeLists.txt"
> +               ((".*cpack.d.*") ""))
> +             #t))))

What does this do? Please add an explanatory comment.

> Subject: [PATCH 3/4] gnu: Add python-pivy.
> 
>     * gnu/packages/python-xyz.scm (python-pivy): New variable.

> +        (snippet
> +          '(begin
> +             (substitute* "CMakeLists.txt"
> +                          (("\\$\\{SoQt_INCLUDE_DIRS}")
> +                           "${Coin_INCLUDE_DIR};${SoQt_INCLUDE_DIRS}"))
> +             #t))))

Origin snippets affect what is returned by `guix build --source
python-pivy`, and are used for correcting very serious bugs in the
source code or for fixing so-called "freedom issues", such as removing
pre-compiled binaries.

We prefer to make other types of changes in custom build phases in a
package's arguments. I'm not sure exactly what this substitution does —
please add a comment — but perhaps it would be more appropriate in a
custom build phase?

> +    (arguments
> +      `(#:tests? #f))

Why are the tests disabled? We aim to make Guix packages pass upstream
test suites, so there should be a reason for skipping them. If there is
no test suite, just add a comment saying so. Same question about the
soqt package.

> Subject: [PATCH 4/4] gnu: freecad correct runtime errors
> 
>     * gnu/packages/engineering.scm (freecad): Update package
>     [inputs]: Move python-pyside-2-tools to native-inputs
>     [inputs]: Add pivy
>     [inputs]: Add qtxmlpatterns
>     [inputs]: Add qtwebkit

Are all of these changes necessary to fix the errors? If not, we prefer
to split the changes up into separate commits. For example, one commit
to update the package, one commit to fix the errors, one commit to
enable some optional feature (e.g. requiring qtwebkit).

If all the changes must be made together, that's fine too.

The commit message should be rewritten, but exactly how depends on
answers to my previous questions. It could be written like this:

------
gnu: FreeCad: Update to 0.18.5-1.7616153.

Fixes *description of bug*.

* gnu/packages/engineering.scm (freecad): Update to 0.18.5-1.7616153.
[inputs]: Add python-pivy, qtxmlpatterns, and qtwebkit. Remove
python-pyside-2-tools.
[native-inputs]: Add python-pyside-2-tools.
------

Can you send a revised patch series?