guix-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[bug#36352] [PATCH] gnu: Add solvespace.


From: Myles English
Subject: [bug#36352] [PATCH] gnu: Add solvespace.
Date: Tue, 3 Sep 2019 01:28:28 +0100

Hi Ludo',

Thanks a lot for your comments, sorry I hadn't replied yet.

On Mon, 2 Sep 2019, 14:24 Ludovic Courtès, <address@hidden> wrote:
Did you have a chance to look into that?  (See below.)

Ludovic Courtès <address@hidden> skribis:

> Hello Myles,
>
> Myles English <address@hidden> skribis:
>
>> * gnu/packages/game-development.scm (solvespace): New variable and

game-development.scm? I think I put it in the wrong place.

>> dependencies on gnu package modules.
>
> The patch LGTM overall.  Here are some comments/questions:
>
>> +(define-public solvespace-3
>> +  (let ((commit "5df53fc59e7f31e265fabd4c15e6601bd3032833")
>> +    (revision "1"))
>> +    (package
>> +      (name "solvespace")
>> +      (version (git-version "3.0" revision commit))
>
>
> Why choose this commit specifically?  ‘git describe’ returns
> “v2.1.rc1-570-g5df53fc”, and in fact there’s no “v3.0” tag, so this
> version string is a bit misleading.
>
> The general policy is to use the latest release, but if there’s a
> compelling argument, we can use another commit; in that case, it’s
> better to have a comment explaining the choice.

It looks as though a longawaited v3.0 release may be imminent, hence my delay in replying to your advice.  I'll wait another week or two before asking if it is going to drop 'soon'.  (I would have tried to justify my choice by referencing the unofficial Debian package, mentioned on the project's github page, using the master branch and calling itself v3.0.)

>> +         (uri (git-reference
>> +               (url "https://github.com/solvespace/solvespace.git")
>> +               (commit commit)
>> +               (recursive? #t)))
>
> Is ‘recursive?’ needed?  If it’s just for the bundled dependencies under
> extlib/ that are not used anyway, perhaps we can omit it?

Some of them are still used (sorry I can't check which ones just now).  I have trivially modified the build system so that if :recursive? is a list it only clones those submodules listed. I can find the patch later but its just a couple of lines.  If this is adopted should may be rename :recursive? or add another parameter?

>> +      (synopsis "Parametric 2D/3D CAD tool")
>> +      (description "Parametric 2D/3D computer aided design (CAD) tool and
>> +constraint-based parametric modeler with simple mechanical simulation
>> +capabilities.")
>
> Could you make it a full sentence and perhaps expound a little bit, as
> per <https://gnu.org/s/guix/manual/en/html_node/Synopses-and-Descriptions.html>?

I'll try and improve this when I make a patch for the true v3.0.

Myles

reply via email to

[Prev in Thread] Current Thread [Next in Thread]