GNU bug report logs - #78233
[PATCH 0/2 electronics-team] Upgrade nextpnr.

Previous Next

Full log

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

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Cayetano Santos <csantosb <at> inventati.org>
Cc: 78233-done <at> debbugs.gnu.org, Gabriel Wicki <gabriel <at> erlikon.ch>,
 Ekaitz Zarraga <ekaitz <at> elenq.tech>
Subject: Re: [bug#78233] [PATCH 2/2] gnu: nextpnr-ice40: Update to 0.8.
Date: Mon, 12 May 2025 16:14:12 +0900

Hi,

Cayetano Santos <csantosb <at> inventati.org> writes:

> n<#secure method=pgpmime mode=sign>
>
> Hi Maxim,
>
> Thanks for all your comments !  They’re (almost) included in v2 series.
>
>>jeu. 08 mai 2025 at 22:07, Maxim Cournoyer <maxim.cournoyer <at> gmail.com> wrote:
>
>> Interesting, I had never seen the opendir/closedir approach, perhaps
>> because the code base in Guix embraces functional rather than imperative
>> style.  It'd be more conventional (in Guix at least) to use (ice-9
>> ftw)'s scandir to list the files and apply for-each to its value, and I
>> think it'd be more succinct too.
>>
>> Something like:
>>
>> (with-directory-excursion "icestorm/examples"
>>                        (for-each (cut invoke "make" "-C" <>)
>>                                  (scandir "."
>>                                           (negate (cut member <> '("." ".."))))))
>
> I just learned about cut existence, thanks again; I understand its utility
> by looking at examples all around the code. No doubt, this is the correct
> alternative.
>
> Unfortunately, I’m unable at this point to use it, I miss the necessary
> skills: I fall in frequent "unbound variable <>" errors that I don’t
> known yet how to handle, I suspect gexps add a extra layer of complexity
> which becomes a barrier to me, sorry for that.
>
>> Hm, this relocation in the same diff as the changes muddled the scope.
>> I thought it was a new entry above, so reviewed it as such.  Could you
>> please split the relocation as a prior commit with your actual changes
>> to review in a 2nd commit on top?  This should give a nicer view, as
>> Gabriel also noted.
>
> Let’s forget the relocation by now, as it complicates things with no
> real utility by itself. We will sort things out later on.

Change clarity/transparency and easing the review is worth the extra
effort, in my opinion.

> Just consider that, in reality, nextpnr replaces former nextpnr-ice40
> (it is strongly based on it); new nextpnr-ice40 is a new package (even
> if it carries the name of previous one).  This is not evident with two commits.
>
> Two commits, without breaking things up, are necessarily somehow
> confusing (and kind of useless, to that matter). To me, the only means
> of achieving a nicer diff view is using a single commit, where one may
> compare new nextpnr with old nextpnr-ice40, at the same time as it becomes
> evident that the new nextpnr-ice40 is an addition.
>
> V2 is in its way.

Thanks for V2, I've used it as the base of nextpnr-ice40 package, for
which I've also added patches, as mentioned previously.  By first
updating nextpnr-ice40 to v0.8 and related changes, and later renaming
it to nextpnr with few adjustments needed, the commit history is as
clear as it can be.  Please have a look/test to make sure everything
still works as intended.

I didn't keep the phase building the examples as the package already has
a test suite, and I didn't see the gain/effort ratio of doing so as
worth it.

-- 
Thanks,
Maxim

Previous Next

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