Subject: [bug#73152] [PATCH 3/6] gnu: Add make-nss.

From: Ian Eure <[email-masked]>
Date: Sun, 16 Feb 2025 10:48:38 -0800

Hi Christopher,

Christopher Baines <[email-masked]> writes:

> Ian Eure <[email-masked]> writes:
>
>> * gnu/packages/nss.scm (make-nss): New variable.
>> NSS builds require time-shifting to their approximate release 
>> date to build
>> repeatably, because it ships with test certificates which 
>> expire.  To avoid
>> duplicating the whole package definition between `nss' and 
>> `nss-rapid', move
>> the bulk of the definition into `make-nss', which accepts a 
>> version, hash, and
>> release date, allowing reuse between the two definitions.
>>
>> Change-Id: Iaab1bb167ceed985a3dcde57f7fe35dce3deaa36
>> ---
>>  gnu/packages/nss.scm | 166 
>>  +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 166 insertions(+)
>
> I'm not sure the refactoring here is overall helpful, I think I
> understand the motivation but I think it would be simpler and 
> more
> readable to stick with the package inheritance approach.
>
> If you just need to change the source, plus the faketime date in
> nss-rapid, but want to avoid replacing the entire check phase, 
> maybe you
> could change the nss package to use an environment variable
> (e.g. GUIX_CHECK_FAKETIME_DATE) for this, and set this 
> environment
> variable in a single phase.
>
> So in nss you'd have:
>
>   (add-before 'check 'set-GUIX_CHECK_FAKETIME_DATE
>     (lambda _
>       (setenv "GUIX_CHECK_FAKETIME_DATE" "2024-01-23")))
>   (replace 'check
>     (lambda* (#:key tests? #:allow-other-keys)
>       ...
>       (invoke #$(if (target-64bit?) "faketime" "datefudge")
>               (getenv "GUIX_CHECK_FAKETIME_DATE") 
>               "./nss/tests/all.sh")))
>
> Then in nss-rapid you'd just do
>
>   (replace 'set-GUIX_CHECK_FAKETIME_DATE
>     (lambda _
>       (setenv "GUIX_CHECK_FAKETIME_DATE" "2024-08-30")))
>
> Maybe there's a more elegant way to share a value between phases 
> in the
> builder, but I think even doing it via an environment variable 
> is still
> preferable than using a procedure to create the package. I've 
> spent many
> hours debugging complex functional and performance related 
> issues caused
> by procedures returning packages, and while it's a powerful 
> tool, it's
> something to be avoided unless necessary.

Thank you very much for the detailed review and suggestion.  I 
like the environment variable approach a lot, and will send an 
updated patch series which uses it.  I agree with you that 
straightforward package definitions are preferable, and this is a 
much simpler approach.

> In terms of how to make this kind of change, I'd split it in to 
> two
> parts. Introducing the environment variable can definately go to 
> the
> core-packages-team branch in my opinion, and the package updates 
> could
> maybe as well, but I'd think of it as two separate patch series.

The split that makes sense to me is to send one series to 
core-packages-team, consisting of: one patch to use an environment 
variable for the release date; a second patch to ungraft nss; and 
a third package updatingd nss to the latest ESR.  I believe each 
patch in this series will cause nss dependents to rebuild, so it 
seems preferable to put those into a single series, rather than 
turn a 15k package build into a 45k one.

Then, after core-packages-team merges, a second patch to master 
which updates nss-rapid to use the environment variable mechainsm. 
Since very little depends on this package, it’s safe to update in 
master any time.

If that sounds good to you, I’ll close this bug and open a new one 
with the first series.

Thank you again for engaging with me on moving forward on this 
work.

  -- Ian