If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add modular NSS builds to TC

RESOLVED FIXED in 3.30

Status

NSS
Test
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: fkiefer, Assigned: ueno)

Tracking

trunk
3.30
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

We should add builds to TC that build NSS in the modular way that RedHat uses.
(Assignee)

Comment 1

a year ago
Created attachment 8791971 [details]
split.tar.gz

I am attaching a set of scripts that split the TC build phase into 3 stages as in Fedora.  To try, extract the tarball under automation/taskcluster/scripts and do the following:

- rm -rf dist* nss*
- hg clone https://hg.mozilla.org/projects/nss
- USE_64=1 ./build_util.sh
- USE_64=1 ./build_softoken.sh
- USE_64=1 ./build_nss.sh

Note that there are dependencies between the stages and the scripts must be called in the above order.
A couple of comments.

It's better to attach suggested changes as patches/text files, so they can be read immediately, without having to download/extract.

I've looked at your attachment. Should there be a tools.sh which isn't contained in the archive?

If we want to integrate it, it should ideally be in a format that can be checked in to the repository. We shouldn't have to download these scripts from a secondary location, they should be part of NSS.

And a question, without having fully read your scripts: Does your script create a secondary copy of all source files, and builds inside the secondary copy?

We have the ability to start an experimental ("try") build inside our continous integration. If you can provide a patch against the NSS repository, I could start a try build.
After adding an attachment, it's usually best to ask a specific person to look at your new attachment and provide feedback. For attached patches, it's possible to click "details", and set the review flag to "?" and enter the email address of the person that you're requesting to review it.
Depends on: 1306319

Updated

a year ago
Assignee: nobody → dueno
Created attachment 8796593 [details] [diff] [review]
tc-modular-builds.patch

This is a patch with the splitting scripts and some adjustments to get it closer to something working.

Updated

a year ago
Blocks: 1280846

Comment 5

11 months ago
Comment on attachment 8796593 [details] [diff] [review]
tc-modular-builds.patch

Review of attachment 8796593 [details] [diff] [review]:
-----------------------------------------------------------------

Minor fixes and suggestions for the comments, LGTM

::: automation/taskcluster/scripts/split.sh
@@ +111,5 @@
> +split_nss() {
> +  nssdir="$1"
> +  dstdir="$2"
> +
> +  # Prepare a source tree only containing files to build nss-softoken:

It should be
# Prepare a source tree only containing files to build nss:

@@ +123,5 @@
> +  #   nss/lib/softoken/dbm        full directory
> +
> +  # Copy everything.
> +  cp -R $nssdir $dstdir
> +

Add # Remove subdirectories that we don't want.

@@ +135,5 @@
> +  # Copy these headers until the upstream bug is accepted
> +  # Upstream https://bugzilla.mozilla.org/show_bug.cgi?id=820207
> +  cp $nssdir/lib/softoken/lowkeyi.h $dstdir/cmd/rsaperf
> +  cp $nssdir/lib/softoken/lowkeyti.h $dstdir/cmd/rsaperf
> +

Add # Copy verref.h which will be needed later during the build phase.
(Assignee)

Comment 6

8 months ago
Created attachment 8828385 [details] [diff] [review]
tc-modular-builds-v2.patch

Thank you for the review.  I have updated the patch addressing the issues rebased against the master.  Also I split the nspr build into a separate phase.

To test, I only did the following:
- cd automation/taskcluster/scripts
- hg clone https://hg.mozilla.org/projects/nss
- USE_64=1 NSS_BUILD_MODULAR=1 ./build.sh

to avoid temporary dependency issues in the current master, you might need to apply a patch:
http://pkgs.fedoraproject.org/cgit/rpms/nss-softokn.git/tree/nss-softokn-basicutil-dependency.patch
and also add "USE_STATIC_LIBS = 1" to nss/cmd/lowhashtest/manifest.mn.
Comment on attachment 8828385 [details] [diff] [review]
tc-modular-builds-v2.patch

Review of attachment 8828385 [details] [diff] [review]:
-----------------------------------------------------------------

When you can fix the issue with softokn this should be good to go.
Can you also attach the patch from [1]? Then we can review and land them together.

[1] http://pkgs.fedoraproject.org/cgit/rpms/nss-softokn.git/tree/nss-softokn-basicutil-dependency.patch

::: automation/taskcluster/scripts/split.sh
@@ +150,5 @@
> +  # depends on internal symbols not exported from the shared library.
> +  sed '/	util_gtest \\/d' $dstdir/gtests/manifest.mn > $dstdir/gtests/manifest.mn-t && mv $dstdir/gtests/manifest.mn-t $dstdir/gtests/manifest.mn
> +
> +  # FIXME: Link to libsoftokn3.so instead of libsoftokn.a
> +  sed 's|\$(DIST)/lib/\$(LIB_PREFIX)softokn\.\$(LIB_SUFFIX)|-lsoftokn3|' $dstdir/gtests/ssl_gtest/manifest.mn > $dstdir/gtests/ssl_gtest/manifest.mn-t && mv $dstdir/gtests/ssl_gtest/manifest.mn-t $dstdir/gtests/ssl_gtest/manifest.mn

This doesn't work. It will link against the system softokn3.so not against the newly build one. This is probably not what you want (and it won't work on TC as there's no NSS installed).
Attachment #8828385 - Flags: review-
Flags: needinfo?(dueno)
(Assignee)

Comment 8

8 months ago
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #7)
> Comment on attachment 8828385 [details] [diff] [review]
> tc-modular-builds-v2.patch
> 
> Review of attachment 8828385 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> When you can fix the issue with softokn this should be good to go.
> Can you also attach the patch from [1]? Then we can review and land them
> together.

OK, I'm attaching those patches now.

> [1]
> http://pkgs.fedoraproject.org/cgit/rpms/nss-softokn.git/tree/nss-softokn-
> basicutil-dependency.patch
> 
> ::: automation/taskcluster/scripts/split.sh
> @@ +150,5 @@
> > +  # depends on internal symbols not exported from the shared library.
> > +  sed '/	util_gtest \\/d' $dstdir/gtests/manifest.mn > $dstdir/gtests/manifest.mn-t && mv $dstdir/gtests/manifest.mn-t $dstdir/gtests/manifest.mn
> > +
> > +  # FIXME: Link to libsoftokn3.so instead of libsoftokn.a
> > +  sed 's|\$(DIST)/lib/\$(LIB_PREFIX)softokn\.\$(LIB_SUFFIX)|-lsoftokn3|' $dstdir/gtests/ssl_gtest/manifest.mn > $dstdir/gtests/ssl_gtest/manifest.mn-t && mv $dstdir/gtests/ssl_gtest/manifest.mn-t $dstdir/gtests/ssl_gtest/manifest.mn
> 
> This doesn't work. It will link against the system softokn3.so not against
> the newly build one. This is probably not what you want (and it won't work
> on TC as there's no NSS installed).

This seems not the case, because a proper -L option to prefer the built one is emitted, according to the build log.  However, I realized that the build succeeds even without linking to that library, so I removed it from the manifest.mn.
Flags: needinfo?(dueno)
(Assignee)

Comment 9

8 months ago
Created attachment 8828864 [details] [diff] [review]
tc-modular-builds-v3.patch

Changed to not touch ssl_gtest/manifest.mn.
Attachment #8828385 - Attachment is obsolete: true
(Assignee)

Comment 10

8 months ago
Created attachment 8828865 [details] [diff] [review]
fix-linking.patch

This patch fixes a couple of link errors when doing modular builds.
(Assignee)

Comment 11

8 months ago
Created attachment 8828869 [details] [diff] [review]
basicutil-HexString2SECItem.patch

This patch fixes a layer violation between basicutil.h and secutil.h, split from: http://pkgs.fedoraproject.org/cgit/rpms/nss-softokn.git/tree/nss-softokn-basicutil-dependency.patch
(Assignee)

Comment 12

8 months ago
Created attachment 8828870 [details] [diff] [review]
basicutil-SSLNamedGroup.patch

This patch fixes another layer violation between basicutil.h and secutil.h, regarding SSLNamedGroup, split from: http://pkgs.fedoraproject.org/cgit/rpms/nss-softokn.git/tree/nss-softokn-basicutil-dependency.patch
Attachment #8796593 - Attachment is obsolete: true
https://hg.mozilla.org/projects/nss/rev/d733c88aa82736cf2722184ddf82a297c00781a0
https://hg.mozilla.org/projects/nss/rev/d24a446e39a623dea48923d07d502706016d53da
https://hg.mozilla.org/projects/nss/rev/4cf932d59e1506dfe16de61da47315911215e841
https://hg.mozilla.org/projects/nss/rev/0106c7de03b8c068a3b1becf6e189344ad9b32a0
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 3.30
You need to log in before you can comment on or make changes to this bug.