diff mbox series

[bug#45573] Correct freecad runtime errors

Message ID TQfDzzG0TKkwRtg4mmmxbY9ptCT7JGE-ij8Nl2xmDj23f7w0rTG3QfZEYfNaq6Mzh_CDNAbtJxKVhTFL4IP1YgQgImlaG4STqV6DdGoauQc=@elenq.tech
State Accepted
Headers show
Series [bug#45573] Correct freecad runtime errors | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue

Commit Message

Ekaitz Zarraga Jan. 1, 2021, 2:37 p.m. UTC 
Hi,

I attach the corrected patch set.
If something is missing please let me know.

Thanks.

Happy new year.

Ekaitz

Comments

Leo Famulari Jan. 4, 2021, 12:13 a.m. UTC | #1 
On Fri, Jan 01, 2021 at 02:37:39PM +0000, Ekaitz Zarraga wrote:
> I attach the corrected patch set.
> If something is missing please let me know.

Thanks for the revisions!

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

> +    (name "coin3D-4")

I changed the name to "coin3D". One can specify the version in the Guix
UI with "coin3D@4" and in code by referring to the variable name,
coin3D-4.

> +             ;; Delete references to packaging tool cpack
> +             (substitute* "CMakeLists.txt"
> +               ((".*cpack.d.*") ""))
> +             #t))))

I still did not understand the reason for this substitution. I tried
building without it and found that the build is broken, so I added this
info to the comment.

Can you report it upstream? It seems like something that all
distributors would benefit from.

> Subject: [PATCH 2/6] gnu: Add soqt.
> 
>     * gnu/packages/qt.scm (soqt): New variable.

I pushed these first two patches as
a5f13705cb9261ab66bdf73d1fb4a832714feb31

Comments on the others to follow...
Leo Famulari Jan. 4, 2021, 12:18 a.m. UTC | #2 
On Fri, Jan 01, 2021 at 02:37:39PM +0000, Ekaitz Zarraga wrote:
> Subject: [PATCH 3/6] gnu: Add python-pivy.
> 
>     * gnu/packages/python-xyz.scm (python-pivy): New variable.

> +      `(#:tests? #f ; Tests are broken

Can you clarify what you mean, and the overall situation with the tests?
Are they actually used upstream?

> +        #:phases
> +        (modify-phases %standard-phases
> +          (add-after 'unpack 'patch-cmake-include-dirs
> +           (lambda _
> +             ;; Patch buildsystem to respect Coin3D include directory
> +             (substitute* "CMakeLists.txt"
> +                          (("\\$\\{SoQt_INCLUDE_DIRS}")
> +                           "${Coin_INCLUDE_DIR};${SoQt_INCLUDE_DIRS}"))

This can probably be fixed with #:configure-flags. I can look into this
before pushing.

> Subject: [PATCH 4/6] gnu: FreeCad: Update to 0.18.5-1.7616153.
> 
> Fixes Draft module import errors
> 
>     * gnu/packages/engineering.scm (freecad): Update to 0.18.5-1.7616153.
>     [inputs]: Add python-pivy.


> Subject: [PATCH 5/6] gnu: freecad: move python-pyside-2-tools to native-inputs
> 
>     * gnu/packages/engineering.scm (freecad):
>     [inputs]: Remove python-pyside-2-tools.
>     [native-inputs]: Add python-pyside-2-tools.

The re-indentation of the package in patch 4/6 is not complete, and I
will squash these two patches before pushing. I have this "ready to go"
in my Git tree.

> Subject: [PATCH 6/6] gnu: freecad: Add qtwebkit input.
> 
>     * gnu/packages/engineering.scm (freecad):
>     [inputs]: Add qtwebkit.

> -       ;; qtwebkit is optional. We remove it currently, because it takes
> -       ;; much time to compile and substitutes are often unavailable
> -       ;;("qtwebkit" ,qtwebkit)
> +       ("qtwebkit" ,qtwebkit)

The comment is still true... I recommend adding a note in the commit
message saying what the new dependency enables.
Ekaitz Zarraga Jan. 4, 2021, 12:01 p.m. UTC | #3 
Hi Leo,

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, January 4, 2021 1:13 AM, Leo Famulari <leo@famulari.name> wrote:


>
> > -               ;; Delete references to packaging tool cpack
> >
> >
> > -               (substitute* "CMakeLists.txt"
> >
> >
> > -                 ((".*cpack.d.*") ""))
> >
> >
> > -               #t))))
> >
> >
>
> I still did not understand the reason for this substitution. I tried
> building without it and found that the build is broken, so I added this
> info to the comment.
>
> Can you report it upstream? It seems like something that all
> distributors would benefit from.


I think it's related with packaging. But I'm not sure neither.
Guys at Nix do the same and remove that line. I tried to leave it
and it doesn't work. I'll try to report upstream.

> > Subject: [PATCH 2/6] gnu: Add soqt.
> >
> >     * gnu/packages/qt.scm (soqt): New variable.
> >
>
> I pushed these first two patches as
> a5f13705cb9261ab66bdf73d1fb4a832714feb31
>
> Comments on the others to follow...


Thanks!
Ekaitz Zarraga Jan. 4, 2021, 12:15 p.m. UTC | #4 
Hi,


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, January 4, 2021 1:18 AM, Leo Famulari <leo@famulari.name> wrote:

> On Fri, Jan 01, 2021 at 02:37:39PM +0000, Ekaitz Zarraga wrote:
>
> > Subject: [PATCH 3/6] gnu: Add python-pivy.
> >
> >     * gnu/packages/python-xyz.scm (python-pivy): New variable.
> >
>
> > -        `(#:tests? #f ; Tests are broken
> >
> >
>
> Can you clarify what you mean, and the overall situation with the tests?
> Are they actually used upstream?

I think they are broken upstream.

When they are run during the guix compilation they report a circular
dependency issue when loading but once the lib is installed i'm able to
import it without issues.

>
> > -          #:phases
> >
> >
> > -          (modify-phases %standard-phases
> >
> >
> > -            (add-after 'unpack 'patch-cmake-include-dirs
> >
> >
> > -             (lambda _
> >
> >
> > -               ;; Patch buildsystem to respect Coin3D include directory
> >
> >
> > -               (substitute* "CMakeLists.txt"
> >
> >
> > -                            (("\\\\$\\\\{SoQt_INCLUDE_DIRS}")
> >
> >
> > -                             "${Coin_INCLUDE_DIR};${SoQt_INCLUDE_DIRS}"))
> >
> >
>
> This can probably be fixed with #:configure-flags. I can look into this
> before pushing.

I tried that and I was unable to solve it that way.
I'm not a CMake expert but I think the problem is that even if CMake finds
Coin3D, it's not taking it in account during the compilation, so it needs
that patch to use it.

>
> > Subject: [PATCH 4/6] gnu: FreeCad: Update to 0.18.5-1.7616153.
> > Fixes Draft module import errors
> >
> >     * gnu/packages/engineering.scm (freecad): Update to 0.18.5-1.7616153.
> >     [inputs]: Add python-pivy.
> >
>
> > Subject: [PATCH 5/6] gnu: freecad: move python-pyside-2-tools to native-inputs
> >
> >     * gnu/packages/engineering.scm (freecad):
> >     [inputs]: Remove python-pyside-2-tools.
> >     [native-inputs]: Add python-pyside-2-tools.
> >
>
> The re-indentation of the package in patch 4/6 is not complete, and I
> will squash these two patches before pushing. I have this "ready to go"
> in my Git tree.
>
> > Subject: [PATCH 6/6] gnu: freecad: Add qtwebkit input.
> >
> >     * gnu/packages/engineering.scm (freecad):
> >     [inputs]: Add qtwebkit.
> >
>
> > -         ;; qtwebkit is optional. We remove it currently, because it takes
> >
> >
> > -         ;; much time to compile and substitutes are often unavailable
> >
> >
> > -         ;;("qtwebkit" ,qtwebkit)
> >
> >
> >
> > -         ("qtwebkit" ,qtwebkit)
> >
> >
>
> The comment is still true... I recommend adding a note in the commit
> message saying what the new dependency enables.

I'm not sure if the comment is true.
I'd like to discuss it, but you can safely discard this change.

The only part that is affected by qtwebkit is the first screen of the
program that shows some examples, links and news. So it's safe to remove
but I'm not sure if the substitutes were unavailable because of this or
because the compilation was failing (it have been broken for a long time).

I'm not sure about how to proceed here. I'm ok with a FreeCad that
is open in a blank screen and shows a couple of warnings on load. I'll
leave the decision of including this patch or not on you guys if you don't
mind.


Thank you for your time,
Ekaitz
Leo Famulari Jan. 4, 2021, 8:12 p.m. UTC | #5 
On Mon, Jan 04, 2021 at 12:15:26PM +0000, Ekaitz Zarraga wrote:
> I think they are broken upstream.
> 
> When they are run during the guix compilation they report a circular
> dependency issue when loading but once the lib is installed i'm able to
> import it without issues.

Thanks for the info. I added it to a comment. It helps reviewers (and
later readers of the package definition) to include bits of info to help
understand why the package definition does something non-standard.

> I tried that and I was unable to solve it that way.
> I'm not a CMake expert but I think the problem is that even if CMake finds
> Coin3D, it's not taking it in account during the compilation, so it needs
> that patch to use it.

Okay.

> The only part that is affected by qtwebkit is the first screen of the
> program that shows some examples, links and news. So it's safe to remove
> but I'm not sure if the substitutes were unavailable because of this or
> because the compilation was failing (it have been broken for a long time).
> 
> I'm not sure about how to proceed here. I'm ok with a FreeCad that
> is open in a blank screen and shows a couple of warnings on load. I'll
> leave the decision of including this patch or not on you guys if you don't
> mind.

Now that I understand that the absence of QtWebKit breaks part of the
FreeCAD interface, I agree that it should be included.

I squashed the patches in a way that seemed appropriate, wrote the
commit messages, and pushed as ed2e0b1b50587a38ad26574585f73979874e56f0
diff mbox series

Patch

From 42a78d572abdddbed86b69de0e87468823bfe8d9 Mon Sep 17 00:00:00 2001
From: Ekaitz Zarraga <ekaitz@elenq.tech>
Date: Fri, 1 Jan 2021 15:22:02 +0100
Subject: [PATCH 4/6] gnu: FreeCad: Update to 0.18.5-1.7616153.

Fixes Draft module import errors

    * gnu/packages/engineering.scm (freecad): Update to 0.18.5-1.7616153.
    [inputs]: Add python-pivy.
---
 gnu/packages/engineering.scm | 41 ++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/gnu/packages/engineering.scm b/gnu/packages/engineering.scm
index e436994492..4bb7f68939 100644
--- a/gnu/packages/engineering.scm
+++ b/gnu/packages/engineering.scm
@@ -2432,28 +2432,21 @@  full programmatic control over your models.")
     (license license:gpl2+)))
 
 (define-public freecad
-  (package
-    (name "freecad")
-    (version "0.18.5")
-    (source
-     (origin
-       (method git-fetch)
-       (uri (git-reference
-             (url "https://github.com/FreeCAD/FreeCAD")
-             (commit version)))
-       (modules '((guix build utils)))
-       (snippet
-        '(begin
-           ;; Fix build with Python 3.8, see
-           ;; <https://tracker.freecadweb.org/view.php?id=4143>.
-           (substitute* "src/Base/swigpyrun.inl"
-             (("PyObject \\*modules = interp->modules;")
-              "PyObject *modules = PyEval_GetBuiltins();"))
-           #t))
-       (file-name (git-file-name name version))
-       (sha256
-        (base32
-         "0r31jzzkamf76l19fb175hhv48irk06fpi8ldxdlr31w8c1ix4aa"))))
+  (let ((commit-ref "7616153b3c31ace006169cdc2fdafab484498858")
+        (revision "1"))
+    (package
+      (name "freecad")
+      (version (git-version "0.18.5" revision commit-ref))
+      (source
+        (origin
+          (method git-fetch)
+          (uri (git-reference
+                 (url "https://github.com/FreeCAD/FreeCAD")
+                 (commit commit-ref)))
+          (file-name (git-file-name name version))
+          (sha256
+            (base32
+              "16965yxnp2pq7nm8z3p0pjkzjdyq62vfrj8j3nk26bwc898czyn2"))))
     (build-system qt-build-system)
     (native-inputs
      `(("doxygen" ,doxygen)
@@ -2479,6 +2472,7 @@  full programmatic control over your models.")
        ("python-pyside-2" ,python-pyside-2)
        ("python-pyside-2-tools" ,python-pyside-2-tools)
        ("python-shiboken-2" ,python-shiboken-2)
+       ("python-pivy" ,python-pivy)
        ("python-wrapper" ,python-wrapper)
        ("qtbase" ,qtbase)
        ("qtsvg" ,qtsvg)
@@ -2546,7 +2540,8 @@  customization.")
       license:lgpl2.1+
       license:lgpl2.0+
       license:gpl3+
-      license:bsd-3))))
+      license:bsd-3)))))
+
 
 (define-public libmedfile
   (package
-- 
2.29.2