[bug#78555] gnu: Add pythonscad

  * gnu/packages/engineering.scm (pythonscad): New variable

Change-Id: I66737d0eaaa7546dcd407d7371166c9b5d5d9b31
---
 gnu/packages/engineering.scm | 77 +++++++++++++++++++++++++++++++++++-
 1 file changed, 75 insertions(+), 2 deletions(-)
  

Hi!

nomike <nomike@nomike.com> writes:

> * gnu/packages/engineering.scm (pythonscad): New variable
>
> Change-Id: I66737d0eaaa7546dcd407d7371166c9b5d5d9b31
> ---
>  gnu/packages/engineering.scm | 77 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 75 insertions(+), 2 deletions(-)
>
> diff --git a/gnu/packages/engineering.scm b/gnu/packages/engineering.scm
> index 8b1332e186..c81f4d5dbe 100644
> --- a/gnu/packages/engineering.scm
> +++ b/gnu/packages/engineering.scm
> @@ -3315,6 +3315,79 @@ (define-public openscad
>        (home-page "https://openscad.org/")
>        (license license:gpl2+))))
>  
> +(define-public pythonscad
> +  (let ((commit "b856456fcf26e089a616e5a9b5d685b8a8f2e2c1")
> +        (version "2025.05.21")
> +        (revision "0"))
> +    (package
> +      (inherit openscad)
> +      (name "pythonscad")
> +      (version (git-version version revision commit))
> +      (source
> +       (origin
> +         (method git-fetch)
> +         (uri (git-reference
> +               (url "https://github.com/pythonscad/pythonscad")
> +               (commit commit)
> +               (recursive? #t)))

Use of recursive? #t should be explained in a comment above; it's
typically used to bundle libraries that we'd rather package separately
and link to, if possible.

> +         (sha256
> +          (base32 "0bjpvj94m3kplpnmnlai7mjx45d5acnqw943w3p1mprg8wrp3ap6"))
> +         (file-name (git-file-name name version))))
> +      (arguments
> +       (list
> +        #:configure-flags
> +        #~(list "-DCMAKE_BUILD_TYPE=Release"
> +                "-DUSE_BUILTIN_OPENCSG=ON"
> +                "-DMANIFOLD_PYBIND=OFF"
> +                "-DMANIFOLD_TEST=OFF"
> +                "-DENABLE_TESTS=OFF"

Why is the test suite disabled?  This also needs an explanation.  We
strive to run test suites in Guix.

> +                "-DEXPERIMENTAL=ON"
> +                "-DSNAPSHOT=ON"

Are these really needed here?  In general, we stick to the default
configuration unless we have specific needs.

> +                "-DENABLE_PYTHON=ON"
> +                (string-append "-DPYTHON_VERSION="
> +                               (string-join (reverse (list-tail (reverse (string-split #$
> +                                                                          (package-version
> +                                                                           (this-package-input
> +                                                                            "python"))
> +                                                                          #\.))
> +                                                                1))
> "."))

You can use #$(version-major+minor (package-version (this-package-input
"python"))), from (guix utils) instead.  Please try to fit under 80
characters maximum width, which is in our coding style guidelines.

> +                "-DUSE_BUILTIN_CLIPPER2=OFF"
> +                (string-append "-DOPENSCAD_VERSION="
> +                               #$version)
> +                (string-append "-DOPENSCAD_COMMIT="
> +                               #$commit)
> +                "-DENABLE_EGL=ON"
> +                "-DENABLE_GLX=ON")
> +        #:phases
> +        #~(modify-phases %standard-phases
> +            (delete 'check)
> +            (add-after 'unpack 'patch-source
> +              (lambda* (#:key inputs #:allow-other-keys)
> +                ;; <https://github.com/openscad/openscad/issues/5877>

It's very nice to have the cross-references to the issues being patched
here!  It'd be even nicer if there was a short explanatory comment
introducing the issue, e.g. "The build system fails to find lib3mf; it
seems it should be using PC_LIB3MF_INCLUDEDIR instead of
PC_LIB3MF_INCLUDE_DIRS (see:
<https://github.com/openscad/openscad/issues/5880>)."  It's more
verbose, but it's nice to summarize the issue to save the reader from
having to follow the link and read the whole conversation to figure it
out (perhaps they work offline too, or the link may disappear in the
future).

> +                (substitute* "cmake/Modules/FindLib3MF.cmake"
> +                  (("PC_LIB3MF_INCLUDE_DIRS")
> +                   "PC_LIB3MF_INCLUDEDIR"))
> +                (substitute* "CMakeLists.txt"
> +                  ;; <https://github.com/openscad/openscad/issues/5880>
> +                  (("target_link_libraries\\(OpenSCAD PRIVATE OpenGL::EGL\\)")
> +                   "      find_package(ECM REQUIRED NO_MODULE)
> +      list(APPEND CMAKE_MODULE_PATH ${ECM_MODULE_PATH})
> +      find_package(EGL REQUIRED)
> +      target_link_libraries(OpenSCAD PRIVATE EGL::EGL)")
> +                  ;; <https://github.com/openscad/openscad/issues/5897>
> +                  (("find_package\\(Nettle 3.4\\)")
> +                   "find_package(Nettle 3.4 REQUIRED)")
> +                  ;; Use the system sanitizers-cmake module.
> +                  (("\\$\\{CMAKE_SOURCE_DIR\\}/submodules/sanitizers-cmake/cmake")
> +                   (string-append (assoc-ref inputs "sanitizers-cmake")
> +                                  "/share/sanitizers-cmake/cmake"))))))))

I'd use (search-input-directory (or native-inputs inputs)
                                "share/sanitizers-cmake/cmake")

The (or native-inputs inputs) is useful when cross-compiling; in that
situation, the native-inputs are in the native inputs, when doing a
straight build (host -> host), the native-inputs are fused into the
inputs.

> +      (inputs (modify-inputs (package-inputs openscad)
> +                (append libjpeg-turbo curl python nettle)))
> +      (home-page "https://openscad.org/")
> +      (description
> +       "A fork of @code{OpenSCAD} that lets you use Python as its native language."))))

Perhaps you could reuse some of the original openscad package
description, to give more information on what this package does, and
append the above to specify how it differs from the original.

Could you please send a v2 with the above comments taken into
consideration?
  

Hi!

Thanks a lot for the detailed feedback. It is really valuable.

On 26.05.25 14:21, Maxim Cournoyer wrote:
> Use of recursive? #t should be explained in a comment above; it's
> typically used to bundle libraries that we'd rather package separately
> and link to, if possible.

I mainly copied this section from the openscad package definition, which 
I updated previously.
I remember that there were some submodules which are exclusively for 
OpenSCAD which was the main reason for keeping them.
Pythonscad is just a fork of OpenSCAD, but that means those submodules 
are now used by two packages, so it makes more sense to split them out 
into their own packages.

I will work on that.

>>   "-DENABLE_TESTS=OFF"

This was a recommendation from one of the OpenSCAD project team members. 
I was having some issues getting the test cases to succeed. Since then I 
have gained a lot more knowledge about OpenSCAD, CMake and guix, I will 
revisit this topic.

>> +                "-DEXPERIMENTAL=ON"

The deal with this flag: OpenSCAD is a 3D design tool where you create 
your object by writing code in a DSL (pythonscad adds python support on 
top of that). There are commands like cube() or sphere() to create objects.
There are some commands though, which the project deems experimental. 
For example roof(), which adds a roof over a 2D polygon. Apart from 
`-DEXPERIMENTAL=ON` during building, users also have to enable those 
experimental language features individually in the preferences of 
OpenSCAD. So I don't really see any harm in having this flag enabled, 
but the benefit for users to having new functions available. As they 
still need to be enabled explicitly in the application preferences, 
users relying on production grade fully tested features only are 
unlikely to bump into this.

I will however, raise a ticket with the upstream project as the 
preferences page only labels them as "Features" and doesn't highlight 
their experimental nature. IMHO it should either be clear from the GUI, 
that those features are experimental or, if they are no longer 
experimental, they should be included in the build even if 
`-DEXPERIMENTAL=OFF`.

The point for pythonscad in particular however is, that the whole 
program is still experimental. So while it might be possible to set 
EXPERIMENTAL to OFF in package openscad, this will not work for pythonscad.

An issue I have with OpenSCAD is that their last release is from January 
2021. Since then the project has evolved a lot, but the devs always 
restrained from creating new release citing that they just need a little 
bit more time to stabilize things. This seems to have been going on for 
a while and I suspect they are knees-deep into feature-creep. As to 
them, the next release is right around the corner, but whether this 
really happen, we'll see.

Meanwhile the 2021.01 release is quit outdated and I've been told a 
number of times by project members to use the nightly builds, as they 
are more modern, have a bunch of really useful new features and the 
project team considers them to be quite stable already.

I'm not really happy with that situation, having a package in guix which 
is based on a git commit and not on an official release, but as far as I 
can tell most people starting with OpenSCAD use the version supplied by 
their OS for a few days and then quickly switch to a nightly from snap 
or flatpak and I've been using a recent snapshot version for months now, 
without issues. So I figured this would still serve our users best.

I will also stay behind updates and will try for the guix packages not 
to fall behind too far.

As for pythonscad: There hasn't been any release yet, so there it's 
anyway the only thing we can do for now.

>> +                "-DSNAPSHOT=ON"

You're right, this is not needed. As the default is set to OFF, I will 
just remove this.

>> +                "-DENABLE_PYTHON=ON"
>> +                (string-append "-DPYTHON_VERSION="
>> +                               (string-join (reverse (list-tail (reverse (string-split #$
>> +                                                                          (package-version
>> +                                                                           (this-package-input
>> +                                                                            "python"))
>> +                                                                          #\.))
>> +                                                                1))
>> "."))
> You can use #$(version-major+minor (package-version (this-package-input
> "python"))), from (guix utils) instead.  Please try to fit under 80
> characters maximum width, which is in our coding style guidelines.

Thanks, I changed the code accordingly.
I have an issue with the column lengths though:
I'm using emacs with aut-fill-mode to edit the source, which will take 
care of not to exceed the 80 column limit. I've also checked things 
manually to make sure.

The GUIX manual about how to submit patches though, says that I should 
run `guix style` before submitting the patch. And if I do that, it undos 
my changes and the code exceeds the 80-column barrier again.

First of all, I'm not sure which has more precedence in this case. I 
guess it's the 80-column rule, but that would then mean that `guix 
style` is buggy and that I should file a bug report, right?

>> +                "-DUSE_BUILTIN_CLIPPER2=OFF"
>> +                (string-append "-DOPENSCAD_VERSION="
>> +                               #$version)
>> +                (string-append "-DOPENSCAD_COMMIT="
>> +                               #$commit)
>> +                "-DENABLE_EGL=ON"
>> +                "-DENABLE_GLX=ON")
>> +        #:phases
>> +        #~(modify-phases %standard-phases
>> +            (delete 'check)
>> +            (add-after 'unpack 'patch-source
>> +              (lambda* (#:key inputs #:allow-other-keys)
>> +                ;;<https://github.com/openscad/openscad/issues/5877>
> It's very nice to have the cross-references to the issues being patched
> here!  It'd be even nicer if there was a short explanatory comment
> introducing the issue, e.g. "The build system fails to find lib3mf; it
> seems it should be using PC_LIB3MF_INCLUDEDIR instead of
> PC_LIB3MF_INCLUDE_DIRS (see:
> <https://github.com/openscad/openscad/issues/5880>)."  It's more
> verbose, but it's nice to summarize the issue to save the reader from
> having to follow the link and read the whole conversation to figure it
> out (perhaps they work offline too, or the link may disappear in the
> future).

I will add more verbose commentary about those upstream tickets. But 
here I again have an issue with the 80 columns rule:

For some files there is only one patch, for some there are multiple. In 
case of multiple patches, I have to put the comment right above the 
patch of course. But this means that the start of the comment is already 
quite far to the right. Especially with those URLS to the upstream 
tickets. it's over 80 columns with the URL alone. I think I'm not 
supposed to put a line-break directly inside the URL, or am I (or use a 
"\" before the line-break)? The whole idea of keeping things below 80 
columns is to increase readability, but if I break an URL in half, it 
would IMHO decrease readability a lot, working against the nature of the 
80-column rule.

Any advice about what I should to here?

>> +                (substitute* "cmake/Modules/FindLib3MF.cmake"
>> +                  (("PC_LIB3MF_INCLUDE_DIRS")
>> +                   "PC_LIB3MF_INCLUDEDIR"))
>> +                (substitute* "CMakeLists.txt"
>> +                  ;;<https://github.com/openscad/openscad/issues/5880>
>> +                  (("target_link_libraries\\(OpenSCAD PRIVATE OpenGL::EGL\\)")
>> +                   "      find_package(ECM REQUIRED NO_MODULE)
>> +      list(APPEND CMAKE_MODULE_PATH ${ECM_MODULE_PATH})
>> +      find_package(EGL REQUIRED)
>> +      target_link_libraries(OpenSCAD PRIVATE EGL::EGL)")
>> +                  ;;<https://github.com/openscad/openscad/issues/5897>
>> +                  (("find_package\\(Nettle 3.4\\)")
>> +                   "find_package(Nettle 3.4 REQUIRED)")
>> +                  ;; Use the system sanitizers-cmake module.
>> +                  (("\\$\\{CMAKE_SOURCE_DIR\\}/submodules/sanitizers-cmake/cmake")
>> +                   (string-append (assoc-ref inputs "sanitizers-cmake")
>> +                                  "/share/sanitizers-cmake/cmake"))))))))
> I'd use (search-input-directory (or native-inputs inputs)
>                                  "share/sanitizers-cmake/cmake")
>
> The (or native-inputs inputs) is useful when cross-compiling; in that
> situation, the native-inputs are in the native inputs, when doing a
> straight build (host -> host), the native-inputs are fused into the
> inputs.
Great suggestion. I will do that.
>> +      (inputs (modify-inputs (package-inputs openscad)
>> +                (append libjpeg-turbo curl python nettle)))
>> +      (home-page"https://openscad.org/")
>> +      (description
>> +       "A fork of @code{OpenSCAD} that lets you use Python as its native language."))))
> Perhaps you could reuse some of the original openscad package
> description, to give more information on what this package does, and
> append the above to specify how it differs from the original.

Yes, makes sense.

It's more likely however, that people read the start of a paragraph 
while skipping the end, rather than the other way around. So maybe it 
would be better to first have a bit of text explaining what sets 
pythonscad appart from OpenSCAD, and following that with parts of the 
the package description of OpenSCAD.

What do you think?

> Could you please send a v2 with the above comments taken into
> consideration?
>
Will do, once all these things are discussed and addressed.

Thanks again for the review!

nomike