diff mbox series

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

Message ID 20240909175540.8156-4-ian@retrospec.tv
State New
Headers
Series [bug#73152,1/6] gnu: Remove nss/fixed. |

Commit Message

Ian Eure Sept. 9, 2024, 5:55 p.m. UTC 
* 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(+)

Comments

Liliana Marie Prikler Sept. 10, 2024, 5:59 p.m. UTC | #1 
Am Montag, dem 09.09.2024 um 10:55 -0700 schrieb Ian Eure:
> * 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
> ---
Note: the explanation should come before the ChangeLog.

Cheers
Christopher Baines Feb. 16, 2025, 2:47 p.m. UTC | #2 
Ian Eure <ian@retrospec.tv> 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.

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.
Ian Eure Feb. 16, 2025, 6:48 p.m. UTC | #3 
Hi Christopher,

Christopher Baines <mail@cbaines.net> writes:

> Ian Eure <ian@retrospec.tv> 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
diff mbox series

Patch

diff --git a/gnu/packages/nss.scm b/gnu/packages/nss.scm
index 60b4b34d4e..b51bebda3d 100644
--- a/gnu/packages/nss.scm
+++ b/gnu/packages/nss.scm
@@ -94,6 +94,172 @@  (define-public nspr
 in the Mozilla clients.")
     (license license:mpl2.0)))
 
+(define* (make-nss #:key version release-date hash)
+  (package
+    (name "nss")
+    ;; IMPORTANT: Also update and test the nss-certs package, which duplicates
+    ;; version and source to avoid a top-level variable reference & module
+    ;; cycle.
+    (version version)
+    (source
+     (origin
+       (method url-fetch)
+       (uri (let ((version-with-underscores
+                   (string-join (string-split version #\.) "_")))
+              (string-append
+               "https://ftp.mozilla.org/pub/mozilla.org/security/nss/"
+               "releases/NSS_" version-with-underscores "_RTM/src/"
+               "nss-" version ".tar.gz")))
+       (sha256
+        (base32 hash))
+       ;; Create nss.pc and nss-config.
+       (patches (search-patches "nss-3.56-pkgconfig.patch"
+                                "nss-getcwd-nonnull.patch"
+                                "nss-increase-test-timeout.patch"))
+       (modules '((guix build utils)))
+       (snippet
+        '(begin
+           ;; Delete the bundled copy of these libraries.
+           (delete-file-recursively "nss/lib/zlib")
+           (delete-file-recursively "nss/lib/sqlite")))))
+    (build-system gnu-build-system)
+    (outputs '("out" "bin"))
+    (arguments
+     (list
+      #:make-flags
+      #~(let ((rpath (string-append "-Wl,-rpath=" #$output "/lib/nss")))
+          (list "-C" "nss"
+                (string-append "PREFIX=" #$output)
+                "NSDISTMODE=copy"
+                "NSS_USE_SYSTEM_SQLITE=1"
+                ;; The gtests fail to compile on riscv64.
+                ;; Skipping them doesn't affect the test suite.
+                #$@(if (target-riscv64?)
+                       #~("NSS_DISABLE_GTESTS=1")
+                       #~())
+                ;; Ensure we are building for the (%current-target-system).
+                #$@(if (%current-target-system)
+                       #~((string-append
+                           "OS_TEST="
+                           (string-take #$(%current-target-system)
+                                        (string-index #$(%current-target-system) #\-)))
+                          (string-append
+                           "KERNEL=" (cond (#$(target-hurd?) "gnu")
+                                           (#$(target-linux?) "linux")
+                                           (else ""))))
+                       #~())
+                #$@(if (%current-target-system)
+                       #~("CROSS_COMPILE=1")
+                       #~())
+                (string-append "NSPR_INCLUDE_DIR="
+                               (search-input-directory %build-inputs
+                                                       "include/nspr"))
+                ;; Add $out/lib/nss to RPATH.
+                (string-append "RPATH=" rpath)
+                (string-append "LDFLAGS=" rpath)))
+      #:modules '((guix build gnu-build-system)
+                  (guix build utils)
+                  (ice-9 ftw)
+                  (ice-9 match)
+                  (srfi srfi-26))
+      #:tests? (not (or (%current-target-system)
+                        ;; Tests take more than 30 hours on some architectures.
+                        (target-riscv64?)
+                        (target-ppc32?)))
+      #:phases
+      #~(modify-phases %standard-phases
+          (replace 'configure
+            (lambda _
+              (setenv "CC" #$(cc-for-target))
+              (setenv "CCC" #$(cxx-for-target))
+              (setenv "NATIVE_CC" "gcc")
+              ;; No VSX on powerpc-linux.
+              #$@(if (target-ppc32?)
+                     #~((setenv "NSS_DISABLE_CRYPTO_VSX" "1"))
+                     #~())
+              ;; Tells NSS to build for the 64-bit ABI if we are 64-bit system.
+              #$@(if (target-64bit?)
+                     #~((setenv "USE_64" "1"))
+                     #~())))
+          (replace 'check
+            (lambda* (#:key tests? #:allow-other-keys)
+              (if tests?
+                  (begin
+                    ;; Use 127.0.0.1 instead of $HOST.$DOMSUF as HOSTADDR for
+                    ;; testing.  The latter requires a working DNS or /etc/hosts.
+                    (setenv "DOMSUF" "localdomain")
+                    (setenv "USE_IP" "TRUE")
+                    (setenv "IP_ADDRESS" "127.0.0.1")
+
+                    ;; This specific test is looking at performance "now
+                    ;; verify that we can quickly dump a database", and
+                    ;; we're not testing performance here (especially
+                    ;; since we're using faketime), so raise the
+                    ;; threshold
+                    (substitute* "nss/tests/dbtests/dbtests.sh"
+                      ((" -lt 5") " -lt 50"))
+
+                    ;; Since the test suite is very lengthy, run the test
+                    ;; suite once, not thrice as done by default, by
+                    ;; selecting only the 'standard' cycle.
+                    (setenv "NSS_CYCLES" "standard")
+
+                    #$@(if (target-64bit?)
+                           '()
+                           ;; The script fails to determine the source
+                           ;; directory when running under 'datefudge' (see
+                           ;; <https://issues.guix.gnu.org/72239>).  Help it.
+                           #~((substitute* "nss/tests/gtests/gtests.sh"
+                                (("SOURCE_DIR=.*")
+                                 (string-append "SOURCE_DIR=" (getcwd) "/nss\n")))))
+
+                    ;; The "PayPalEE.cert" certificate expires every six months,
+                    ;; leading to test failures:
+                    ;; <https://bugzilla.mozilla.org/show_bug.cgi?id=609734>.  To
+                    ;; work around that, set the time to roughly the release date.
+                    (invoke #$(if (target-64bit?) "faketime" "datefudge")
+                            #$release-date "./nss/tests/all.sh"))
+                  (format #t "test suite not run~%"))))
+          (replace 'install
+            (lambda* (#:key outputs #:allow-other-keys)
+              (let* ((out (assoc-ref outputs "out"))
+                     (bin (string-append (assoc-ref outputs "bin") "/bin"))
+                     (inc (string-append out "/include/nss"))
+                     (lib (string-append out "/lib/nss"))
+                     (obj (match (scandir "dist" (cut string-suffix? "OBJ" <>))
+                            ((obj) (string-append "dist/" obj)))))
+                ;; Install nss-config to $out/bin.
+                (install-file (string-append obj "/bin/nss-config")
+                              (string-append out "/bin"))
+                (delete-file (string-append obj "/bin/nss-config"))
+                ;; Install nss.pc to $out/lib/pkgconfig.
+                (install-file (string-append obj "/lib/pkgconfig/nss.pc")
+                              (string-append out "/lib/pkgconfig"))
+                (delete-file (string-append obj "/lib/pkgconfig/nss.pc"))
+                (rmdir (string-append obj "/lib/pkgconfig"))
+                ;; Install other files.
+                (copy-recursively "dist/public/nss" inc)
+                (copy-recursively (string-append obj "/bin") bin)
+                (copy-recursively (string-append obj "/lib") lib)))))))
+    (inputs (list sqlite zlib))
+    (propagated-inputs (list nspr))               ;required by nss.pc.
+    (native-inputs (list perl                     ;for tests
+                         (if (target-64bit?) libfaketime datefudge)
+                         which))
+
+    ;; The NSS test suite takes around 48 hours on Loongson 3A (MIPS) when
+    ;; another build is happening concurrently on the same machine.
+    (properties '((timeout . 216000)))  ;60 hours
+
+    (home-page "https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS")
+    (synopsis "Network Security Services")
+    (description
+     "Network Security Services (@dfn{NSS}) is a set of libraries designed to
+support cross-platform development of security-enabled client and server
+applications.  Applications built with NSS can support SSL v2 and v3, TLS,
+PKCS #5, PKCS #7, PKCS #11, PKCS #12, S/MIME, X.509 v3 certificates, and other
+security standards.")
+    (license license:mpl2.0)))
 
 ;; nss should track ESRs, but currently doesn't.  3.102.1 is the current ESR.