From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:470:142:3::10]:33650) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1i4wgx-0000YW-Up for guix-patches@gnu.org; Mon, 02 Sep 2019 20:29:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1i4wgw-0008Vp-AR for guix-patches@gnu.org; Mon, 02 Sep 2019 20:29:03 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:50278) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1i4wgw-0008Vg-80 for guix-patches@gnu.org; Mon, 02 Sep 2019 20:29:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1i4wgw-0005Pt-1g for guix-patches@gnu.org; Mon, 02 Sep 2019 20:29:02 -0400 Subject: [bug#36352] [PATCH] gnu: Add solvespace. Resent-Message-ID: MIME-Version: 1.0 References: <20190624122710.22874-1-myles@tdma.co> <87r2783w83.fsf@gnu.org> <87v9ua96h9.fsf@gnu.org> In-Reply-To: <87v9ua96h9.fsf@gnu.org> From: Myles English Date: Tue, 3 Sep 2019 01:28:28 +0100 Message-ID: Content-Type: multipart/alternative; boundary="00000000000087a60105919b2b2d" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Ludovic =?UTF-8?Q?Court=C3=A8s?= Cc: 36352@debbugs.gnu.org, Myles English --00000000000087a60105919b2b2d Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Ludo', Thanks a lot for your comments, sorry I hadn't replied yet. On Mon, 2 Sep 2019, 14:24 Ludovic Court=C3=A8s, wrote: > Did you have a chance to look into that? (See below.) > > Ludovic Court=C3=A8s skribis: > > > Hello Myles, > > > > Myles English 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? =E2=80=98git describe=E2=80=99 re= turns > > =E2=80=9Cv2.1.rc1-570-g5df53fc=E2=80=9D, and in fact there=E2=80=99s no= =E2=80=9Cv3.0=E2=80=9D tag, so this > > version string is a bit misleading. > > > > The general policy is to use the latest release, but if there=E2=80=99s= a > > compelling argument, we can use another commit; in that case, it=E2=80= =99s > > 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 =E2=80=98recursive?=E2=80=99 needed? If it=E2=80=99s just for the b= undled 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 > --00000000000087a60105919b2b2d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi=C2=A0Ludo',

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

On Mon, 2 Sep 2019, 14:24 Ludovic Court=C3=A8s, <ludo@gnu.org> wrote:
Did you have a chance to look into that?=C2=A0 (See below.)<= br>
Ludovic Court=C3=A8s <ludo@gnu.org> skribis:

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

game-development.sc= m? I think I put it in the wrong place.
<= span style=3D"font-family:sans-serif">
>> dependencies on gnu package modules.
>
> The patch LGTM overall.=C2=A0 Here are some comments/questions:
>
>> +(define-public solvespace-3
>> +=C2=A0 (let ((commit "5df53fc59e7f31e265fabd4c15e6601bd30328= 33")
>> +=C2=A0 =C2=A0 (revision "1"))
>> +=C2=A0 =C2=A0 (package
>> +=C2=A0 =C2=A0 =C2=A0 (name "solvespace")
>> +=C2=A0 =C2=A0 =C2=A0 (version (git-version "3.0" revisi= on commit))
>
>
> Why choose this commit specifically?=C2=A0 =E2=80=98git describe=E2=80= =99 returns
> =E2=80=9Cv2.1.rc1-570-g5df53fc=E2=80=9D, and in fact there=E2=80=99s n= o =E2=80=9Cv3.0=E2=80=9D tag, so this
> version string is a bit misleading.
>
> The general policy is to use the latest release, but if there=E2=80=99= s a
> compelling argument, we can use another commit; in that case, it=E2=80= =99s
> better to have a comment explaining the choice.
=

It looks as though a lo= ngawaited v3.0 release may be imminent, hence my delay in replying to your = advice.=C2=A0 I'll wait another week or two before asking if it is goin= g to drop 'soon'.=C2=A0 (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.)

>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(uri (git= -reference
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(url "= ;https://github.com/solvespace/solvespace.git= ")
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(commit co= mmit)
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(recursive= ? #t)))
>
> Is =E2=80=98recursive?=E2=80=99 needed?=C2=A0 If it=E2=80=99s 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 one= s just now).=C2=A0 I have trivially modified the build system=C2=A0so that if=C2=A0:recursive?= is a list it only clones tho= se submodules listed. I can find the patch later but its just a couple of l= ines.=C2=A0 If this is adopted should may be rename :recursive? or add anot= her parameter?

>> +=C2=A0 =C2=A0 =C2=A0 (synopsis "Parametric 2D/3D CAD tool&qu= ot;)
>> +=C2=A0 =C2=A0 =C2=A0 (description "Parametric 2D/3D computer= aided design (CAD) tool and
>> +constraint-based parametric modeler with simple mechanical simula= tion
>> +capabilities.")
>
> Could you make it a full sentence and perhaps expound a little bit, as=
> per <htt= ps://gnu.org/s/guix/manual/en/html_node/Synopses-and-Descriptions.html&= gt;?

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

Myles
--00000000000087a60105919b2b2d--