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