GNU bug report logs - #56354
[PATCH] gnu: engineering: Add candle.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 56354 in the body.
You can then email your comments to 56354 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: "Artyom V. Poptsov" <poptsov.artyom <at> gmail.com>
To: guix-patches <at> gnu.org
Subject: [PATCH] gnu: engineering: Add candle.
Date: Sat, 02 Jul 2022 12:47:39 +0300

[Message part 1 (text/plain, inline)]

Hello,

this patch adds Candle[1] CNC controller.

[0001-gnu-engineering-Add-candle.patch (text/x-diff, attachment)]

[Message part 3 (text/plain, inline)]

Thanks,

- Artyom

References:
1. https://github.com/Denvi/Candle

-- 
Artyom "avp" Poptsov <poptsov.artyom <at> gmail.com>
Home page: https://memory-heap.org/~avp/
CADR Hackerspace co-founder: https://cadrspace.ru/
GPG: D0C2 EAC1 3310 822D 98DE  B57C E9C5 A2D9 0898 A02F

[signature.asc (application/pgp-signature, inline)]

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: "Artyom V. Poptsov" <poptsov.artyom <at> gmail.com>
To: guix-patches <at> gnu.org
Subject: Re: [PATCH] gnu: engineering: Add candle.
Date: Mon, 04 Jul 2022 06:36:21 +0300

[Message part 1 (text/plain, inline)]

Hello,

I found out that the patch I've sent was broken in two ways: firstly, it
contains only part of my changes; secondly, Candle fails to store its
settings as it tries to store it in the same directory where the binary
stored.

Here's the updated patch.

[0001-gnu-engineering-Add-candle.patch (text/x-diff, attachment)]

[Message part 3 (text/plain, inline)]

Thanks

- Artyom

-- 
Artyom "avp" Poptsov <poptsov.artyom <at> gmail.com>
Home page: https://memory-heap.org/~avp/
CADR Hackerspace co-founder: https://cadrspace.ru/
GPG: D0C2 EAC1 3310 822D 98DE  B57C E9C5 A2D9 0898 A02F

[signature.asc (application/pgp-signature, inline)]

Message #11 received at 56354 <at> debbugs.gnu.org (full text, mbox):

From: Jean Pierre De Jesus DIAZ <me <at> jeandudey.tech>
To: "56354 <at> debbugs.gnu.org" <56354 <at> debbugs.gnu.org>
Subject: Review
Date: Tue, 05 Jul 2022 12:16:01 +0000

>+    (arguments
>+     `(#:tests? #f                      ; no tests.
>+       #:phases (modify-phases %standard-phases

Can be changed to use G-Expressions:

(arguments
  (list #:tests? #f                       ;; no tests.
        #:phases
        #~(modify-phases %standard-phases
            ...)))

See, https://guix.gnu.org/manual/en/html_node/G_002dExpressions.html

>+                  (add-after 'fix-sources 'fix-application-settings-path

Doesn't depend on 'fix-sources, so it's fine to add after 'unpack.

>+                  (replace 'configure
>+                    (lambda* (#:key outputs #:allow-other-keys)
>+                      (let ((out (assoc-ref outputs "out")))
>+                        (chdir "src")
>+                        (invoke "qmake"))))

It may also be a good idea to set `QMAKE_CC' variable for cross-compilation,
like:

`(invoke "qmake" (string-append "QMAKE_CC=" #$(cc-for-target)))'

And also test cross-compilation:

./pre-inst-env guix build candle \
                          --keep-failed \
                          --target=aarch64-linux-gnu

>+                  (add-after 'configure 'fix-makefile
>+                    (lambda _
>+                      (substitute* "Makefile"
>+                        (("-pipe -Z7") "-pipe")
>+                        (("LFLAGS.*=.*DEBUG .*OPT:REF -Wl,-O1")
>+                         "LFLAGS        = -Wl,-O1"))))

Could this instead be replaced on the `candle.pro' file?

—
Jean-Pierre De Jesus DIAZ

Message #14 received at submit <at> debbugs.gnu.org (full text, mbox):

From: "Artyom V. Poptsov" <poptsov.artyom <at> gmail.com>
To: guix-patches <at> gnu.org
Subject: Re: [PATCH] gnu: engineering: Add candle.
Date: Sat, 09 Jul 2022 16:43:56 +0300

[Message part 1 (text/plain, inline)]

Hello Jean Pierre De Jesus DIAZ,

I almost overlooked your message from debbugs as it wasn't delivered to
my mailbox.  Thanks for the patch review!

> Can be changed to use G-Expressions:

Done.

> >+                  (add-after 'fix-sources 'fix-application-settings-path
> Doesn't depend on 'fix-sources, so it's fine to add after 'unpack.

Done.

> It may also be a good idea to set `QMAKE_CC' variable for cross-compilation,
> like:
>
> `(invoke "qmake" (string-append "QMAKE_CC=" #$(cc-for-target)))'

Done.

But I wasn't able to run cross-compilation with the command you
provided:

--8<---------------cut here---------------start------------->8---
$ ./pre-inst-env guix build --keep-failed --target=aarch64-linux-gnu candle
guix build: error: gnu/packages/freedesktop.scm:1921:2: perl-file-mimeinfo <at> 0.29: build system `perl' does not support cross builds
--8<---------------cut here---------------end--------------->8---

It seems to me from the message that the issue is not in the candle
package itself.

> >+                  (add-after 'configure 'fix-makefile
> >+                    (lambda _
> >+                      (substitute* "Makefile"
> >+                        (("-pipe -Z7") "-pipe")
> >+                        (("LFLAGS.*=.*DEBUG .*OPT:REF -Wl,-O1")
> >+                         "LFLAGS        = -Wl,-O1"))))
>
> Could this instead be replaced on the `candle.pro' file?

I fixed it by using the latest commit from the 'master' branch instead
of 1.2b tag.

Please find the updated patch attached.

[0001-gnu-engineering-Add-candle.patch (text/x-diff, attachment)]

[Message part 3 (text/plain, inline)]

Thanks,

- Artyom

-- 
Artyom "avp" Poptsov <poptsov.artyom <at> gmail.com>
Home page: https://memory-heap.org/~avp/
CADR Hackerspace co-founder: https://cadrspace.ru/
GPG: D0C2 EAC1 3310 822D 98DE  B57C E9C5 A2D9 0898 A02F

[signature.asc (application/pgp-signature, inline)]

Message #17 received at 56354 <at> debbugs.gnu.org (full text, mbox):

From: Jean Pierre De Jesus DIAZ <me <at> jeandudey.tech>
To: "56354 <at> debbugs.gnu.org" <56354 <at> debbugs.gnu.org>,
 "poptsov.artyom <at> gmail.com" <poptsov.artyom <at> gmail.com>
Subject: [PATCH] gnu: engineering: Add candle.
Date: Mon, 11 Jul 2022 11:55:52 +0000

Hello Artyom,

>It seems to me from the message that the issue is not in the candle
>package itself.

Probably one of the dependencies can't (currently) be cross-compiled,
anyway, thanks for setting the QMAKE_CC variable in the qmake invocation,
once perl build system gains cross-compilation (if possible) then it
would be easier to cross-compile later without modifications to the
package.

>+                            (lambda* (#:key outputs #:allow-other-keys)
>+                              (let ((out (assoc-ref outputs "out")))
>+                                (chdir "src")
>+                                (invoke "qmake"
>+                                        (string-append "QMAKE_CC="
>+                                                       #$(cc-for-target))))))

The `out' variable is not used, so the let can be safely removed and the lambda
simplified, also instead of `chdir', `with-directory-excursion' could be used,
but's a matter of preference (don't know if one or other style is preferred
inside GNU Guix).

For example:

(lambda _
  (with-directory-excursion "src"
    (invoke "qmake"
      (string-append "QMAKE_CC="
                     #$(cc-for-target)))))

>+                          (replace 'install
>+                            (lambda* (#:key outputs #:allow-other-keys)
>+                              (let ((out (assoc-ref outputs "out")))
>+                                (install-file "Candle"
>+                                              (string-append out "/bin"))))))))

The `out' binding can be also replaced by `#$output', e.g.:


(lambda* _
  (install-file "Candle"
                (string-append #$output "/bin")))

Other than that the package definition looks good to me, and did a quick pass
over the Candle source code to check that it doesn't contain any malware.

Only a bundled font is present (src/fonts/Ubuntu-Regular.tff), but that one is
not provided by GNU Guix (don't know the specific reasons, but got added then
removed) so no need to replace it with provided ones.

—
Jean-Pierre De Jesus DIAZ

Message #20 received at 56354 <at> debbugs.gnu.org (full text, mbox):

From: "Artyom V. Poptsov" <poptsov.artyom <at> gmail.com>
To: Jean Pierre De Jesus DIAZ <me <at> jeandudey.tech>
Cc: "56354 <at> debbugs.gnu.org" <56354 <at> debbugs.gnu.org>
Subject: Re: [PATCH] gnu: engineering: Add candle.
Date: Mon, 11 Jul 2022 21:39:03 +0300

[Message part 1 (text/plain, inline)]

Hello Jean Pierre De Jesus DIAZ.

> The `out' variable is not used, so the let can be safely removed and the lambda
> simplified, also instead of `chdir', `with-directory-excursion' could be used,
> but's a matter of preference (don't know if one or other style is preferred
> inside GNU Guix).

Done.

> The `out' binding can be also replaced by `#$output', e.g.:

Done.

I also tried to use 'with-directory-excursion' instead of 'chdir' but
Candle build fails with it, so I kept 'chdir' version.

> Other than that the package definition looks good to me, and did a quick pass
> over the Candle source code to check that it doesn't contain any malware.

> Only a bundled font is present (src/fonts/Ubuntu-Regular.tff), but that one is
> not provided by GNU Guix (don't know the specific reasons, but got added then
> removed) so no need to replace it with provided ones.

Thanks again for the patch review!

Here's my updated patch.

[0001-gnu-engineering-Add-candle.patch (text/x-diff, attachment)]

[Message part 3 (text/plain, inline)]

- Artyom

-- 
Artyom "avp" Poptsov <poptsov.artyom <at> gmail.com>
Home page: https://memory-heap.org/~avp/
CADR Hackerspace co-founder: https://cadrspace.ru/
GPG: D0C2 EAC1 3310 822D 98DE  B57C E9C5 A2D9 0898 A02F

[signature.asc (application/pgp-signature, inline)]

Message #25 received at 56354-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: "Artyom V. Poptsov" <poptsov.artyom <at> gmail.com>
Cc: Jean Pierre De Jesus DIAZ <me <at> jeandudey.tech>,
 "56354 <at> debbugs.gnu.org" <56354-done <at> debbugs.gnu.org>
Subject: Re: bug#56354: [PATCH] gnu: engineering: Add candle.
Date: Tue, 12 Jul 2022 23:59:50 +0200

[Message part 1 (text/plain, inline)]

Hi,

"Artyom V. Poptsov" <poptsov.artyom <at> gmail.com> skribis:

> From 7aac50730febeb95d0f7a4ab1cbb6e7dfb632553 Mon Sep 17 00:00:00 2001
> From: "Artyom V. Poptsov" <poptsov.artyom <at> gmail.com>
> Date: Thu, 23 Jun 2022 23:02:40 +0300
> Subject: [PATCH] gnu: engineering: Add candle.
>
> * gnu/packages/engineering.scm (candle): New variable.

Applied with the changes below.  Note that the license is GPLv3+ because
nothing says it’s “version 3 only”.  I tweaked the description as per
<https://guix.gnu.org/manual/devel/en/html_node/Synopses-and-Descriptions.html>.

Thank you, and thanks Jean-Pierre for reviewing!

Ludo’.

[Message part 2 (text/x-patch, inline)]

diff --git a/gnu/packages/engineering.scm b/gnu/packages/engineering.scm
index e1297c0f85..dd594af7f8 100644
--- a/gnu/packages/engineering.scm
+++ b/gnu/packages/engineering.scm
@@ -3721,7 +3721,8 @@ (define-public candle
                           (add-after 'unpack 'fix-application-settings-path
                             (lambda _
                               (substitute* "src/frmmain.cpp"
-                                (("qApp->applicationDirPath\\(\\) \\+ \"\\/settings\\.ini\"")
+                                (("\
+qApp->applicationDirPath\\(\\) \\+ \"\\/settings\\.ini\"")
                                  "QDir::homePath() + \"/.config/candle.ini\""))))
                           (replace 'configure
                             (lambda _
@@ -3736,9 +3737,10 @@ (define-public candle
       (home-page "https://github.com/Denvi/Candle")
       (synopsis "GRBL controller with G-Code visualizer")
       (description
-       "GRBL controller application with G-Code visualizer written in Qt.
+       "Candle is a GRBL controller application with a visualizer for G-Code,
+the @acronym{CNC, computer numerical control} programming language.
 
-Supported functions:
+Supported functions include:
 
 @itemize
 @item Controlling GRBL-based cnc-machine via console commands, buttons on
@@ -3747,5 +3749,4 @@ (define-public candle
 @item Loading, editing, saving and sending of G-code files to CNC-machine.
 @item Visualizing G-code files.
 @end itemize")
-      (license license:gpl3))))
-
+      (license license:gpl3+))))

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.