mbox series

[bug#59761,0/2] Add u-boot-ts7970-q-2g-1000mhz-c.

Message ID 20221202052903.10475-1-maxim.cournoyer@gmail.com
Headers show
Series Add u-boot-ts7970-q-2g-1000mhz-c. | expand

Message

Maxim Cournoyer Dec. 2, 2022, 5:29 a.m. UTC 
Hello!

This makes our make-u-boot-package procedure more flexible, and builds on it
to add a new U-Boot bootloader package for an i.MX6 embedded board.

Thanks,

Maxim Cournoyer (2):
  gnu: make-u-boot-package: Add a u-boot argument.
  gnu: Add u-boot-ts7970-q-2g-1000mhz-c.

 gnu/packages/bootloaders.scm | 238 +++++++++++++++++++++++------------
 1 file changed, 155 insertions(+), 83 deletions(-)


base-commit: 4781f0458de7419606b71bdf0fe56bca83ace910

Comments

Ricardo Wurmus Dec. 29, 2022, 7:33 p.m. UTC | #1 
Hi Maxim,

there seems to be some overlap between this and
https://issues.guix.gnu.org/60224.  Looking just at v4 I only have one
comment.

In your substitute* replacements it’s better not to use string-append.
You can include real line breaks in a string and escape line breaks with
\.  This is preferable to gluing strings together.  For something as
long as the replacements in this package consider using a patch file
instead.  This has the added advantage of failing the build when the
patch cannot be applied cleanly.

The rest looks good to me.
Maxim Cournoyer Jan. 4, 2023, 4:17 a.m. UTC | #2 
Hi Ricardo,

Ricardo Wurmus <rekado@elephly.net> writes:

> Hi Maxim,
>
> there seems to be some overlap between this and
> https://issues.guix.gnu.org/60224.

Yes, I ended up splitting my changes focusing on u-boot in #60224, which
should be reviewed before and blocking this change here, which is based
on it.

> Looking just at v4 I only have one
> comment.
>
> In your substitute* replacements it’s better not to use string-append.

Oh?  Why is this so?  There must be hundreds of string-append occurences
used in such place, so I'm curious.

> You can include real line breaks in a string and escape line breaks with
> \.  This is preferable to gluing strings together.

OK, I guess this is your rationale for the above comment (cleaner).

> For something as
> long as the replacements in this package consider using a patch file
> instead.  This has the added advantage of failing the build when the
> patch cannot be applied cleanly.

I agree that a patch would be most suitable here, especially that if
something breaks, if would likely be silent (unlikely to be caught at
build time).  I'll extract this as a patch.

> The rest looks good to me.

OK.  I'll await your comments on #60224, which is awaiting feedback
post-rework based on your earlier feedback.

PS: I had also missed that email; please keep me in CC in all your
replies :-).
Maxim Cournoyer Jan. 4, 2023, 5:34 a.m. UTC | #3 
Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

>> For something as
>> long as the replacements in this package consider using a patch file
>> instead.  This has the added advantage of failing the build when the
>> patch cannot be applied cleanly.
>
> I agree that a patch would be most suitable here, especially that if
> something breaks, if would likely be silent (unlikely to be caught at
> build time).  I'll extract this as a patch.

After re-diving into the code, I opted to kept it as a substitution
given it affects multiple config files in the same way, and would make a
large, redundant patch.  I cleaned it up per your suggestions (see v5).
Ricardo Wurmus Jan. 4, 2023, 7:35 a.m. UTC | #4 
> PS: I had also missed that email; please keep me in CC in all your
> replies :-).

Oh, that’s odd.  I’m replying “from scratch” going just by the bug
number in issues.guix.gnu.org; it doesn’t expose your email address in a
convenient way, so I usually just grab the issue number and write an
email.

Shouldn’t debbugs Cc you when receiving comments on your patch
submission?
Maxim Cournoyer Jan. 4, 2023, 2:46 p.m. UTC | #5 
Hi Ricardo,

Ricardo Wurmus <rekado@elephly.net> writes:

>> PS: I had also missed that email; please keep me in CC in all your
>> replies :-).
>
> Oh, that’s odd.  I’m replying “from scratch” going just by the bug
> number in issues.guix.gnu.org; it doesn’t expose your email address in a
> convenient way, so I usually just grab the issue number and write an
> email.
>
> Shouldn’t debbugs Cc you when receiving comments on your patch
> submission?

It would be nice if it did, but I don't think it does.  Also, the
'X-Debbugs-Cc' header used in teams.scm doesn't seem to cause an actual
CC; I think it'd just cause someone not already subscribed to the
guix-patches mailing list to be sent an email.  I think it'd be better
to simply use git-send-email's '--cc', or both.

My Gnus email filter is based on the Return-Path:

--8<---------------cut here---------------start------------->8---
	   (nnimap-split-methods
	    ;; Filter guix mailing lists based on Return
	    (("list.\\1" "^Return-Path: <\\(.*\\)-bounces.*@\\(non\\)?gnu.org>")
             ("list.\\1" "^Return-Path: <\\(.*\\)-bounces.*@lists.denx.de>")
             ("list.\\1" "^Return-Path: <\\(.*\\)-owner@vger.kernel.org>")
	     ("INBOX" "")))
--8<---------------cut here---------------end--------------->8---

I think when Cc: is used in an email, it causes the Return-Path to be
that of the person sending the email rather than mailman's email, which
is what the above filter expects.