unofficial mirror of guix-devel@gnu.org 
 help / color / mirror / code / Atom feed
From: John Kehayias <john.kehayias@protonmail.com>
To: "Clément Lassieur" <clement@lassieur.org>
Cc: Christopher Baines <mail@cbaines.net>, guix-devel@gnu.org
Subject: Re: branch master updated: gnu: Add passff.
Date: Tue, 31 Oct 2023 02:48:09 +0000	[thread overview]
Message-ID: <87a5rzv76k.fsf@protonmail.com> (raw)
In-Reply-To: <87a5s2bxeg.fsf@lassieur.org>

Hi Clément,

On Sat, Oct 28, 2023 at 05:05 PM, Clément Lassieur wrote:

> On Sat, Oct 28 2023, Christopher Baines wrote:
>
>> This passff-host package looks a bit odd to me, one thing to mention is
>> that guix show says it has no dependencies, but I don't think that's
>> correct:
>>
>>   ./pre-inst-env guix show passff-host
>>   name: passff-host
>>   version: 1.2.3
>>   outputs:
>>   + out: everything
>>   systems: x86_64-linux mips64el-linux aarch64-linux powerpc64le-linux riscv64-linux
>>   + i686-linux armhf-linux i586-gnu powerpc-linux
>>   dependencies:
>
> I imagine it's a bug in `guix show`?  As doc says:
>
>    • Gexps carry information about the packages or derivations they
>      refer to, and these dependencies are automatically added as inputs
>      to the build processes that use them.
>

Right, it uses gexps but I think here the better and more explicit
style would be to use inputs/native-inputs. Then instead of
referencing directly like #$<package-variable-name> use
#$(this-package-input "package-name") to get the store path. This I
think is clearer and I believe better allows for inheritance,
input-rewriting, and so on.

Feel free for anyone else to chime in on this point, I'm always
looking to learn to improve my own packaging and review, but this is
what I understand is preferred when possible.

>> Was this change sent as a patch to guix-patches?
>
> No it wasn't.

The mantra I've heard, and agree with, is that the
trivial-build-system is anything but trivial. Not saying it wasn't the
best choice here, or has anything to do with the above points, but
thought it worth mentioning for anyone else.

But this is also why I think it would have been better to have it go
through review. I see there's been several followup commits to improve
the style and fix things which also could have been avoided. Not a
huge deal perhaps, but I would err on the side of review for something
like this.

Of course, thanks for the contribution!

John



  parent reply	other threads:[~2023-10-31  2:49 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <169826674567.18633.11885829962706141651@vcs2.savannah.gnu.org>
2023-10-28 14:35 ` branch master updated: gnu: Add passff Christopher Baines
2023-10-28 15:05   ` Clément Lassieur
2023-10-28 17:53     ` Kaelyn
2023-10-30 14:23       ` Clément Lassieur
2023-11-01  3:33         ` Clément Lassieur
2023-10-31  2:48     ` John Kehayias [this message]
2023-10-31 19:33       ` Clément Lassieur
2023-11-01  3:36         ` Clément Lassieur
2023-11-03 18:46     ` G-expressions and chroot environment? (was Re: branch master updated: gnu: Add passff.) Simon Tournier
2023-11-06 15:19       ` Simon Tournier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://guix.gnu.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87a5rzv76k.fsf@protonmail.com \
    --to=john.kehayias@protonmail.com \
    --cc=clement@lassieur.org \
    --cc=guix-devel@gnu.org \
    --cc=mail@cbaines.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).