Message ID | 3e28337e7ead18b49e5e2e99d0e3d1d22e3f9d47.1695673614.git.pinoaffe@gmail.com |
---|---|
State | New |
Headers | show |
Series | [bug#66199] gnu: librepcb: Update to 1.0.0. | expand |
Hello, Em 25/09/2023 17:27, pinoaffe escreveu: > * gnu/packages/engineering.scm (librepcb): Update to 1.0.0. Thank you. The commit message should also mention the switch to cmake-build-system, the "not overriding" of the configure phase, the new inputs and the test skip. I have a few more comments below. > --- > gnu/packages/engineering.scm | 29 +++++++++++++---------------- > 1 file changed, 13 insertions(+), 16 deletions(-) > > diff --git a/gnu/packages/engineering.scm b/gnu/packages/engineering.scm > index c2846f0bda..f12f4ad59d 100644 > --- a/gnu/packages/engineering.scm > +++ b/gnu/packages/engineering.scm > @@ -2260,33 +2260,30 @@ (define-public freehdl > (define-public librepcb > (package > (name "librepcb") > - (version "0.1.5") > + (version "1.0.0") > (source > (origin > (method url-fetch) > (uri (string-append "https://download.librepcb.org/releases/" > version "/librepcb-" version "-source.zip")) > (sha256 > - (base32 "0smp1p7wnrj0vh4rmz1cr2krfawc2lzx0pbzmgyay7xdp6jxympr")))) > - (build-system gnu-build-system) > + (base32 "02qfwyhdq1pklb5gkwn3rbsdhwvcgiksd21swaphz3kw6s4p9i8v")))) > + (build-system cmake-build-system) > (inputs > - (list qtbase-5 qtsvg-5 zlib)) > + (list qtbase-5 > + qtsvg-5 > + qtdeclarative-5 > + qtquickcontrols2-5 > + zlib > + opencascade-occt > + glu > + fontconfig)) Inputs should be sorted alphabetically. > (native-inputs > (list qttools-5 ; for lrelease > unzip)) The comment about lrelease can be removed as lrelease is not being manually invoked anymore, and the native inputs list can be squashed in one line. > (arguments > - `(#:phases > - (modify-phases %standard-phases > - (replace 'configure > - (lambda* (#:key inputs outputs #:allow-other-keys) > - (mkdir-p "build") > - (chdir "build") > - (let ((lrelease (search-input-file inputs "/bin/lrelease")) > - (out (assoc-ref outputs "out"))) > - (invoke "qmake" > - (string-append "QMAKE_LRELEASE=" lrelease) > - (string-append "PREFIX=" out) > - "../librepcb.pro"))))))) > + ;; There is no cmake test target > + `(#:tests? #f)) > (home-page "https://librepcb.org/") > (synopsis "Electronic Design Automation tool") > (description "LibrePCB is @dfn{Electronic Design Automation} (EDA) > > base-commit: 445a0359083388b5ee686e6e855f94a3aac5f79c There are options in LibrePCB's CMakeLists.txt to unvendor some dependencies: https://github.com/LibrePCB/LibrePCB/blob/9edb6ede393e5b48785f95252f81a027db4b718a/CMakeLists.txt#L51 The only dependencies we do not have is dxflib and fontobene-qt5. It failed to find muparser and polyclipping ('clipper' package in Guix), I don't know why. It would be best to unvendor as many dependencies as possible. Could you send an updated patch? Optionally, you can try to package fontobene-qt5 and have cmake find muparser and polyclipping. Vinicius
Hi, thank you for your review! Vinicius Monego <monego@posteo.net> writes: > The commit message should also mention the switch to > cmake-build-system, the "not overriding" of the configure phase, the > new inputs and the test skip.. I wasn't sure how to phrase / format this, is what I arrived at OK? > Inputs should be sorted alphabetically. Done > The comment about lrelease can be removed as lrelease is not being > manually invoked anymore, and the native inputs list can be squashed > in one line. Done > There are options in LibrePCB's CMakeLists.txt to unvendor some > dependencies: > https://github.com/LibrePCB/LibrePCB/blob/9edb6ede393e5b48785f95252f81a027db4b718a/CMakeLists.txt#L51 I unvendored all of the mentioned dependencies apart from dxflib (I couldn't get it to build). > The only dependencies we do not have is dxflib and fontobene-qt5. It > failed to find muparser and polyclipping ('clipper' package in Guix), > I don't know why. Adding pkg-config as a native dependency seems to have done the trick > It would be best to unvendor as many dependencies as possible. Could > you send an updated patch? Optionally, you can try to package > fontobene-qt5 and have cmake find muparser and polyclipping. I'll send some updated patches in a minute Kind regards, pinoaffe
Em 26/09/2023 09:08, pinoaffe escreveu: > Hi, Hi! > thank you for your review! > > Vinicius Monego <monego@posteo.net> writes: >> The commit message should also mention the switch to >> cmake-build-system, the "not overriding" of the configure phase, the >> new inputs and the test skip.. > I wasn't sure how to phrase / format this, is what I arrived at OK? You can check the logs of e.g. commit 19617735df2b1af3b169d8153ae543ad3e0fc1a1 for reference. >> Inputs should be sorted alphabetically. > Done > 'googletest' should be a native input, it's a test requirement only. [...] > I'll send some updated patches in a minute > > Kind regards, > pinoaffe [Comment on the LibrePCB update patch specifically]: This v2 is much better, thanks. While investigating the build I also found a few more things that could be improved: - The bundles that were unvendored can be deleted in a source snippet (see e.g. the mixxx package). - LibrePCB builds a test executable at tests/unittests/librepcb-unittests. Instead of 'make test', that file could be run manually when overriding the check phase. - The license list should be updated as some of the bundles listed there are not being provided anymore, or even better let it be only gpl3+ because that's the license of the final product. - I got two merge conflicts on 06dc36ffb7cde821a4762b299d1c95b3788ba110, please rebase it to the latest commit. If the tests are too problematic that's fine, I can merge this patch with the other changes later this week. Vinicius
Thanks again! Vinicius Monego <monego@posteo.net> writes: > Em 26/09/2023 09:08, pinoaffe escreveu: >> Vinicius Monego <monego@posteo.net> writes: >>> The commit message should also mention the switch to >>> cmake-build-system, the "not overriding" of the configure phase, the >>> new inputs and the test skip.. >> I wasn't sure how to phrase / format this, is what I arrived at OK? > You can check the logs of e.g. commit > 19617735df2b1af3b169d8153ae543ad3e0fc1a1 for reference. Ah yes, I'll try to replicate that > 'googletest' should be a native input, it's a test requirement only. oh yes, of course, thanks! > [Comment on the LibrePCB update patch specifically]: This v2 is much > better, thanks. While investigating the build I also found a few more > things that could be improved: > > - The bundles that were unvendored can be deleted in a source snippet > (see e.g. the mixxx package). Done > - LibrePCB builds a test executable at > tests/unittests/librepcb-unittests. Instead of 'make test', that > file could be run manually when overriding the check phase. Done > - The license list should be updated as some of the bundles listed > there are not being provided anymore, or even better let it be only > gpl3+ because that's the license of the final product. I removed all of the licenses and license comments that were no longer relevant, but there's still some non-GPL3+ code involved, so I figured I'd keep those licenses around. > - I got two merge conflicts on > 06dc36ffb7cde821a4762b299d1c95b3788ba110, please rebase it to the > latest commit. Done > If the tests are too problematic that's fine, I can merge this patch > with the other changes later this week. > > Vinicius Kind regards, pinoaffe
diff --git a/gnu/packages/engineering.scm b/gnu/packages/engineering.scm index c2846f0bda..f12f4ad59d 100644 --- a/gnu/packages/engineering.scm +++ b/gnu/packages/engineering.scm @@ -2260,33 +2260,30 @@ (define-public freehdl (define-public librepcb (package (name "librepcb") - (version "0.1.5") + (version "1.0.0") (source (origin (method url-fetch) (uri (string-append "https://download.librepcb.org/releases/" version "/librepcb-" version "-source.zip")) (sha256 - (base32 "0smp1p7wnrj0vh4rmz1cr2krfawc2lzx0pbzmgyay7xdp6jxympr")))) - (build-system gnu-build-system) + (base32 "02qfwyhdq1pklb5gkwn3rbsdhwvcgiksd21swaphz3kw6s4p9i8v")))) + (build-system cmake-build-system) (inputs - (list qtbase-5 qtsvg-5 zlib)) + (list qtbase-5 + qtsvg-5 + qtdeclarative-5 + qtquickcontrols2-5 + zlib + opencascade-occt + glu + fontconfig)) (native-inputs (list qttools-5 ; for lrelease unzip)) (arguments - `(#:phases - (modify-phases %standard-phases - (replace 'configure - (lambda* (#:key inputs outputs #:allow-other-keys) - (mkdir-p "build") - (chdir "build") - (let ((lrelease (search-input-file inputs "/bin/lrelease")) - (out (assoc-ref outputs "out"))) - (invoke "qmake" - (string-append "QMAKE_LRELEASE=" lrelease) - (string-append "PREFIX=" out) - "../librepcb.pro"))))))) + ;; There is no cmake test target + `(#:tests? #f)) (home-page "https://librepcb.org/") (synopsis "Electronic Design Automation tool") (description "LibrePCB is @dfn{Electronic Design Automation} (EDA)