1300109 - Add modular NSS builds to TC

Status: RESOLVED FIXED

(NSS :: Test, defect)

Description

[Franziskus Kiefer [:franziskus]]

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

Comment 1

[Daiki Ueno [:ueno]]

Attachment: 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.

Comment 2

[Kai Engert (:KaiE:)]

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.

Comment 3

[Kai Engert (:KaiE:)]

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.

Comment 4

[Franziskus Kiefer [:franziskus]]

Attachment: tc-modular-builds.patch

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

Comment 5

[Elio Maldonado]

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.

Comment 6

[Daiki Ueno [:ueno]]

Attachment: 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 7

[Franziskus Kiefer [:franziskus]]

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).

Comment 8

[Daiki Ueno [:ueno]]

(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.

Comment 9

[Daiki Ueno [:ueno]]

Attachment: tc-modular-builds-v3.patch

Changed to not touch ssl_gtest/manifest.mn.

Comment 10

[Daiki Ueno [:ueno]]

Attachment: fix-linking.patch

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

Comment 11

[Daiki Ueno [:ueno]]

Attachment: 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

Comment 12

[Daiki Ueno [:ueno]]

Attachment: 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

Comment 13

[Franziskus Kiefer [:franziskus]]

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