Closed Bug 772144 Opened 12 years ago Closed 11 years ago

Run the NSS test suite on ARM/Android

Categories

(NSS :: Test, defect)

Product:
NSS
Component:
Test
Version:
3.14
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
3.14.2

People

(Reporter: KaiE, Assigned: KaiE)

References

(Blocks 2 open bugs,
URL
)

Details

Attachments

(6 files, 5 obsolete files)

Patch v21
30.93 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
Additional changes, including changes needed to support building on Windows
21.69 KB, patch
rrelyea
: review-
KaiE
: feedback-
Details | Diff | Splinter Review
patch v21 - adjusted
30.27 KB, patch
Details | Diff | Splinter Review
bustage fix for older make
1008 bytes, patch
Details | Diff | Splinter Review
windows-testing-fix
933 bytes, patch
Details | Diff | Splinter Review
requested comment
1.08 KB, patch
Details | Diff | Splinter Review 
We'd like to find a way to run the NSS test suite on the ARM/Android platform.
This patch: 

1) makes changes to NSS can build android cross from Linux. You need to install the android NDK first, then set ANDROID_NDK to the path of the NDK, as well as BUILD_ANDROID to tell the build you want to build an android system rather than the native system. nsinstall will still be build natively, shlibsign will be built for android, but the actual signing will happen with the installed nss tools on your system.

2) Install the tests onto your android device. This uses the ssh daemon supplied by the sshdroid app (see google play). You will need to set the ANDROID_ADDR address to tell the system where the tablet is on the net. Install happens through a dedicated target.

3) Run the tests. You can launch the tests from the host once they are installed. Again you need sshdroid installed. This also happens from a make target.

4) Fetch the tests results. You can retrieve the results directory, again with a make target.

NOTES about tests:
The test suite will not run on a FAT file system. Both sqlite and some db function tests will fail.
The FIPS tests will not run because dladdr does not return a full path for implicitly loaded libraries. It is currently disabled here.
Awesome work!

Maybe Mike already knows how to solve the problems with SQLite on a FAT filesystem and/or the dladdr problem.
For dladdr, the only way is to use a different dynamic linker. You can't really do that in standalone nss.
The sqlite and the db test failures are due to the inability to change permissions in files in the FAT filesystem. My guess is the mozilla version of sqlite already addresses the sqlite issue. The db tests, there isn't much that can be done. The tests tries to create a readonly directory and test to see NSS fails with the right error code when it tries to create a new database in that directory. Obviously if you can't actually set the directory to readonly, NSS will be successful in creating a new database and the test will fail. I've solved this by running the tests in the home directory for sshdroid, which is a normal linux file system.

The lack dladdr only affects fips mode (you can't go into fips mode because you can't find the shared library and .chk files). I've solved this by turning off the fips tests.
Update patch.
Attachment #641302 - Attachment is obsolete: true
Attachment #641587 - Flags: superreview?(wtc)
Attachment #641587 - Flags: review?(bsmith)
Patch description:

Source changes for the build.
dbm/src/mktemp.c - don't explicitly include extern reference to errno, depend on errno.h
security/nss/lib/freebl/unix_rand.c - android doesn't include a sysinfo function.
security/nss/lib/util/secasn1t.h - android doesn't export data symbols

Makefile tool changes for build.
Android.mk - create a new OS_ARCH for android. This makefile simply includes the Linux.mk
Linux.mk - Android changes from Linux:
  1. Android doesn't use pthreads.
  2. Set android compiler and defines. (NO_SYSINFO (see above); NO_FORK_CHECK (requires
 pthreads); no fdatasync, use fsync (used by SQLITE); ANDROID (used by secasn1t.h)
  2. Remove _glibc tag for Android.
  3. Android headers aren't ansi (see existing comment in Linux.mk)
  4. Android doesn't need/support NO_DEPEND or LOWHASH.
arch.mk - if BUILD_ANDROID is set, override OS_TEST OS_ARCH and OS_RELEASE. NOTE: this
 assumes we are building arm Android. I think OS_TEST would work as an override. I didn't
 see any other platforms in the android_ndk, however, so I'm not so worried about this.
security/nss/Makefile - 
  1. Changes to tell NSPR to build android.
  2. Make targets to install tests, run test, and fetch the results.
  NOTE: The install changes creates a new $(DIST)/platform.cfg which contains variables the tests
  normally get from 'make'. Android doesn't have a 'make' command, so we build this file on the
  host. See init.sh changes for how it's included.
security/nss/cmd/shlibsign/Makefile - Changes to handle the cross build case.
security/nss/cmd/shlibsign/sign.sh - remaining changes for the cross build case. Uses the local
 systems copy of shlibsign. NOTE: we probably need code for other Linux platforms like Debian and
 Unbuntu.

Test changes:
security/nss/tests/cert/cert.sh - Allow us to turn off the FIPS related tests in cert.sh with
 an environment variable.
security/tests/chains/chains.sh - android doesn't have pushd or popd, so just start 
 httpserve isn a subsell with the correct directory
security/nss/tests/common/init.sh - Turn off unfriendly things for android.
 1) make doesn't exist on android, so if OBJDIR is set, and the platform.cfg exists,
 source that. then only look for make if we are missing a variable we need set.
 2) On android, don't switch to the system path. The system versions of commands are,
 in general, less functional that the busybox versions (rm doesn't understand -rf, for instance).
 Also skip the perl step. Perl isn't on android by default.
 3) Andoid local host names don't have a Domain, skip creating a separate DOMSUF
 4) silences an error message, we don't have permission to set ulimit on android.
 check for make if we
Attachment #641587 - Attachment is patch: true
Attachment #641587 - Attachment mime type: application/x-troff-man → text/plain
It helps to know the wiki page where Bob documented how to run this.
https://wiki.mozilla.org/NSS:Android
Depends on: 777699
Depends on: 777702
Given that it works on newer NSS, maybe I don't care.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Kai, did you mean to WONTFIX this bug, or a different one? I am not sure what your comment 8 means.
Yes, I intended to wontfix bug 777702 instead, reopening.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Attached patch v3 (obsolete) — — Splinter Review 
I suggest a few changes.

(a)
Use an ?= assignment for ANDROID_PORT, allowing the environment to override the script's default (this is helpful if using a device with root permission, on such devices the default port is 22.)

(b)
Bob's patch has make target android_get_result
which is documented on the wiki page.

In addition there is a variation target android_get_results ("s" appended).
Unless Bob tells me this is there for a reason, I think this is an oversight and can be removed.

(c)
I propose to use gzip when creating the tar archives, because CPU is faster than network.

(d)
Fixed "reult" typos -> "result"


I'm attaching an updated patch with these changes.
I successfully used this patch on Android 4.1
Comment on attachment 641587 [details] [diff] [review]
Build Android arm using NSS build tools -v 2

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

I didn't review init.sh.  I suggest some changes below.  Thanks.

::: dbm/src/mktemp.c
@@ +96,2 @@
>  	extern int errno;                    
>  #endif

I believe we can simply remove this manual declaration of errno.
This file includes "winfile.h", which includes <stdlib.h>, and MSDN
says <stdlib.h> declares errno:
http://msdn.microsoft.com/en-us/library/t3ayayh1%28v=VS.80%29.aspx

::: security/coreconf/Android.mk
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +include $(CORE_DEPTH)/coreconf/Linux.mk

I wonder how Mozilla is building NSS for Android.  Perhaps Mozilla
is treating Android as Linux?

::: security/coreconf/Linux.mk
@@ +37,5 @@
> +ifndef INTERNAL_TOOLS
> +	CC = $(ANDROID_CC) --sysroot=$(ANDROID_SYSROOT)
> +	DEFAULT_COMPILER=$(ANDROID_PREFIX)-gcc
> +	ARCHFLAG = --sysroot=$(ANDROID_SYSROOT)
> +	DEFINES += -DNO_SYSINFO -Dfdatasync=fsync -DNO_FORK_CHECK -DANDROID

fdatasync is only used by sqlite, so -Dfdatasync=fsync should
be moved to lib/sqlite/config.mk.  I think we should submit
a patch to sqlite upstream.  Is __linux__ defined on Android?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/sqlite/sqlite3.c&rev=1.6&mark=24409-24410#24403

@@ +38,5 @@
> +	CC = $(ANDROID_CC) --sysroot=$(ANDROID_SYSROOT)
> +	DEFAULT_COMPILER=$(ANDROID_PREFIX)-gcc
> +	ARCHFLAG = --sysroot=$(ANDROID_SYSROOT)
> +	DEFINES += -DNO_SYSINFO -Dfdatasync=fsync -DNO_FORK_CHECK -DANDROID
> +	CROSS_BUILD = 1

Our build systems already have a variable named CROSS_COMPILE:
http://mxr.mozilla.org/security/search?string=CROSS_COMPILE

We should try to use the same variable, if possible.

@@ +131,2 @@
>  STANDARDS_CFLAGS	= -ansi -D_POSIX_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE
> +endif

Maybe we should simply remove the STANDARDS_CFLAGS variable.
I believe -ansi was added when we misunderstood what it means.

::: security/nss/Makefile
@@ +201,5 @@
> +	sftp -o Port=$(ANDROID_PORT) -o CheckHostIP=no -b $(DIST)/android.sftp $(ANDROID_ADDR) 
> +	ssh -p $(ANDROID_PORT) -o CheckHostIP=no $(ANDROID_ADDR)  'cd nsstest ; $$HOME/bin/rm -rf dist security ;  $$HOME/bin/tar xvf nss-android.tar'
> +
> +
> +android_suite: nss_build_all android_install android_run_tests android_get_result

It seems wrong to add the android_install, android_run_tests, android_get_result
makefile targets.  I think we should add a shell script in mozilla/security/nss/tests
for running all.sh on Android.

::: security/nss/cmd/shlibsign/sign.sh
@@ +29,5 @@
> +    # use a native version 
> +    if [ -x /usr/lib64/nss/unsupported-tools/shlibsign ]; then
> +	/usr/lib64/nss/unsupported-tools/shlibsign -v -i ${5}
> +    else
> +	/usr/lib/nss/unsupported-tools/shlibsign -v -i ${5}

These pathnames seem specific to certain Linux distributions.

::: security/nss/lib/freebl/unix_rand.c
@@ +367,2 @@
>      struct sysinfo si;
>      if (sysinfo(&si) == 0) {

I wonder why Mozilla doesn't need this change.
Hi Mike (glandium), do you know the answer to Wan-Teh's question?
Thanks

(In reply to Wan-Teh Chang from comment #13)
> ::: security/coreconf/Android.mk
> @@ +2,5 @@
> > +
> > +include $(CORE_DEPTH)/coreconf/Linux.mk
> 
> I wonder how Mozilla is building NSS for Android.  Perhaps Mozilla
> is treating Android as Linux?
(In reply to Kai Engert (:kaie) from comment #14)
> Hi Mike (glandium), do you know the answer to Wan-Teh's question?
> Thanks
> 
> (In reply to Wan-Teh Chang from comment #13)
> > ::: security/coreconf/Android.mk
> > @@ +2,5 @@
> > > +
> > > +include $(CORE_DEPTH)/coreconf/Linux.mk
> > 
> > I wonder how Mozilla is building NSS for Android.  Perhaps Mozilla
> > is treating Android as Linux?

We basically set the following (additional) flags:
OS_RELEASE=2.6
OS_PTHREAD=
STANDARDS_CFLAGS=-std=gnu89
ARCHFLAG="$(CFLAGS) -DCHECK_FORK_GETPID -DRTLD_NOLOAD=0 -include $(ABS_topsrcdir)/security/manager/android_stub.h"

In the set of flags we set everywhere, there's OS_ARCH, and it has a value of Linux in Android case.

android_stub.h does this:
#define _SYS_SYSINFO_H_

#include <sys/cdefs.h>
#include <linux/kernel.h>

#define sysinfo(foo) -1


(In reply to Wan-Teh Chang from comment #13)
> ::: security/nss/lib/freebl/unix_rand.c
> @@ +367,2 @@
> >      struct sysinfo si;
> >      if (sysinfo(&si) == 0) {
> 
> I wonder why Mozilla doesn't need this change.

Because we avoid patching nss, and have a workaround.
(In reply to Mike Hommey [:glandium] from comment #15)
> We basically set the following (additional) flags:
> OS_RELEASE=2.6

We probably don't need that anymore, btw.
(In reply to Wan-Teh Chang from comment #13)
> ::: security/coreconf/Linux.mk
> @@ +37,5 @@
> > +ifndef INTERNAL_TOOLS
> > +   CC = $(ANDROID_CC) --sysroot=$(ANDROID_SYSROOT)
> > +   DEFAULT_COMPILER=$(ANDROID_PREFIX)-gcc
> > +   ARCHFLAG = --sysroot=$(ANDROID_SYSROOT)
> > +   DEFINES += -DNO_SYSINFO -Dfdatasync=fsync -DNO_FORK_CHECK -DANDROID
> 
> fdatasync is only used by sqlite, so -Dfdatasync=fsync should
> be moved to lib/sqlite/config.mk.  

I understand you want
  ifdef BUILD_ANDROID
  ifndef INTERNAL_TOOLS
          DEFINES += -Dfdatasync=fsync
  endif
  endif
to be added to sqlite


> I think we should submit a patch to sqlite upstream.

Who should do it?


> Is __linux__ defined on Android?

Yes. We are crosscompiling. I used a file containing:

#ifdef __linux__
#error yes defined
#else
#error no, not defined
#endif

$ /home/nssdroid/android-ndk-r8b/toolchains/arm-linux-androideabi-4.4.3/prebuilt/linux-x86/bin/arm-linux-androideabi-gcc --sysroot=/home/nssdroid/android-ndk-r8b/platforms/android-/arch-arm -c -DNO_SYSINFO -Dfdatasync=fsync -DNO_FORK_CHECK -DANDROID  testdef.c

testdef.c:2:2: error: #error yes defined


> Our build systems already have a variable named CROSS_COMPILE:
> http://mxr.mozilla.org/security/search?string=CROSS_COMPILE
> 
> We should try to use the same variable, if possible.

I'll change it, to see what happens.


> @@ +131,2 @@
> >  STANDARDS_CFLAGS   = -ansi -D_POSIX_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE
> > +endif
> 
> Maybe we should simply remove the STANDARDS_CFLAGS variable.
> I believe -ansi was added when we misunderstood what it means.

Ok, but we'll have to test what happens.


> It seems wrong to add the android_install, android_run_tests,
> android_get_result
> makefile targets.  I think we should add a shell script in
> mozilla/security/nss/tests
> for running all.sh on Android.

On the other hand, we have been asked to change the NSS build system, in order to make it possible to build on machine 1 and run the tests on machine 2. Bob had argued we already have that in the test he made.

I propose we introduce a new, universal makefile target for the packaging, only,
and use an android script for testing as Wan-Teh suggested.


> ::: security/nss/cmd/shlibsign/sign.sh
> @@ +29,5 @@
> > +    # use a native version 
> > +    if [ -x /usr/lib64/nss/unsupported-tools/shlibsign ]; then
> > +   /usr/lib64/nss/unsupported-tools/shlibsign -v -i ${5}
> > +    else
> > +   /usr/lib/nss/unsupported-tools/shlibsign -v -i ${5}
> 
> These pathnames seem specific to certain Linux distributions.

Indeed, we cannot do it like this.

Either we must skip the signing,
or, if signing is required,
then we must perform a native build of NSS in addition.

Given that Bob said he excluded the FIPS tests, I propose we omit the signing step for this initial work.
Depends on: 784316
The Nexus 7 is sitting idle at home until we make some progress here.
Blocks: 761987
Comment on attachment 641587 [details] [diff] [review]
Build Android arm using NSS build tools -v 2

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

::: security/coreconf/Android.mk
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +include $(CORE_DEPTH)/coreconf/Linux.mk

See bug 556443, where we hacked Mozilla's build system to build NSS on Android.

See http://mxr.mozilla.org/mozilla-central/source/security/build/Makefile.in#206
and http://mxr.mozilla.org/mozilla-central/source/security/manager/android_stub.h

Based on what I read in those patches, we only support building Fennec on Linux, so I believe we're using the Linux build configuration.

We should remove the Gecko hacks for getting NSS to build on Android. I filed bug 815912 for that.

::: security/coreconf/Linux.mk
@@ +38,5 @@
> +	CC = $(ANDROID_CC) --sysroot=$(ANDROID_SYSROOT)
> +	DEFAULT_COMPILER=$(ANDROID_PREFIX)-gcc
> +	ARCHFLAG = --sysroot=$(ANDROID_SYSROOT)
> +	DEFINES += -DNO_SYSINFO -Dfdatasync=fsync -DNO_FORK_CHECK -DANDROID
> +	CROSS_BUILD = 1

Yes, CROSS_COMPILE is already used by Gecko when it decides to pass various flags to NSS's build.

::: security/nss/Makefile
@@ +201,5 @@
> +	sftp -o Port=$(ANDROID_PORT) -o CheckHostIP=no -b $(DIST)/android.sftp $(ANDROID_ADDR) 
> +	ssh -p $(ANDROID_PORT) -o CheckHostIP=no $(ANDROID_ADDR)  'cd nsstest ; $$HOME/bin/rm -rf dist security ;  $$HOME/bin/tar xvf nss-android.tar'
> +
> +
> +android_suite: nss_build_all android_install android_run_tests android_get_result

I agree with Wan-Teh about moving these makefile rules to a shell script within tests, or even integrating this logic within all.sh.

::: security/nss/lib/freebl/unix_rand.c
@@ +367,2 @@
>      struct sysinfo si;
>      if (sysinfo(&si) == 0) {

For Android builds, we #define sysinfo(foo) -1 in android_stub.h.

If it is OK to avoid all the calls to sysinfo for seeding the PRNG, then why do we ever bother doing it? Perhaps we should just remove all the sysinfo calls and hard fail on getting entropy from the other sources like /dev/[u]random?

::: security/nss/tests/cert/cert.sh
@@ +1455,5 @@
>  cert_ssl 
>  cert_smime_client        
> +if [ -z "NSS_TEST_DISABLE_FIPS" ]; then
> +    cert_fips
> +fi

It would be good to document why we can't do FIPS on Android.

::: security/nss/tests/common/init.sh
@@ +369,4 @@
>              ;;
>      esac
>  
> +    if [ -z "${DOMSUF}" -a "${OS_ARCH}" != "ANDROID" ]; then

We should do this on Windows too. It has the same problem.
(In reply to Kai Engert (:kaie) from comment #17)
> Either we must skip the signing, or, if signing is required,
> then we must perform a native build of NSS in addition.
> 
> Given that Bob said he excluded the FIPS tests, I propose we omit the
> signing step for this initial work.

I agree that it makes sense to skip the signing until we have the FIPS tests running. We can do FIPS mode in a separate follow-up bug. And, in that bug, we can also consider the possibility of making a host version of NSS a build prerequisite just like the NDK is a build prerequisite.
(In reply to Brian Smith (:bsmith) from comment #19)
> Comment on attachment 641587 [details] [diff] [review]
> Build Android arm using NSS build tools -v 2
> 
> Review of attachment 641587 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/coreconf/Android.mk
> @@ +2,5 @@
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +include $(CORE_DEPTH)/coreconf/Linux.mk
> 
> See bug 556443, where we hacked Mozilla's build system to build NSS on
> Android.
> 
> See
> http://mxr.mozilla.org/mozilla-central/source/security/build/Makefile.in#206
> and
> http://mxr.mozilla.org/mozilla-central/source/security/manager/android_stub.h
> 
> Based on what I read in those patches, we only support building Fennec on
> Linux, so I believe we're using the Linux build configuration.

We actually support building Fennec on OSX.
Attached patch Patch v10 (obsolete) — — Splinter Review 
This works on a Android 4.2.1 tablet.

Just a backup for now. Tomorrow I'll comment on the status and reply to earlier comments.
Assignee: nobody → kaie
Attachment #687568 - Attachment description: Patch v1 → Patch v10
Attachment #687568 - Attachment is obsolete: true
Brian, regarding the following items from your comment 19, your questions are outside the scope of this bug. Let's get Android working first, without introducing side effects for other platforms. IMHO your concerns and change requests for other platforms should be done in a separate bug.

(In reply to Brian Smith (:bsmith) from comment #19)
> 
> If it is OK to avoid all the calls to sysinfo for seeding the PRNG, then why
> do we ever bother doing it? Perhaps we should just remove all the sysinfo
> calls and hard fail on getting entropy from the other sources like
> /dev/[u]random?
> 
> .....
>
> >      esac
> >  
> > +    if [ -z "${DOMSUF}" -a "${OS_ARCH}" != "ANDROID" ]; then
> 
> We should do this on Windows too. It has the same problem.
Attached patch Patch v15 (obsolete) — — Splinter Review 
Addressing open questions and review comments:


(a)
Wan-Teh and Brian had complained that we pollute the primary NSS makefile
with Android specific commands.

But moving them to a script would be more complicated,
because we need many variables that are only available while executing
"make".

As a compromise, I propose we simply move those commands to a 
separate directory.

I've created subdirectory tests/remote, as a general purpose directory
for executing tests on a remote system, and I've moved the Android
specific makefile targets to that directory.

In addition, I've also moved makefile target package_for_testing
which I had added in bug 784316.

I've adjusted the target to hopefully work in other remote-testing
scenarios, too, regardless of the platform.
(It can be further improved once we have the need to use it.)


(b)
> shlibsign will be built for android, but the actual signing will happen
> with the installed nss tools on your system.

Because the earlier approach relied on local binaries, I've changed
the runtests script to execute the signing on Android.

That means, we don't need native buildsystem binaries of NSS,
as Brian had suggested.

However, because FIPS testing still doesn't work (as Bob had said in the
initial comment), I've kept FIPS testing disabled for now.

I've addressed Brian's request to add a comment that explains
why it's disabled.


(c)
- removed errno as suggested by Wan-Teh
- removed -ansi from STANDARD_CFLAGS
- changed to use CROSS_COMPILE


(d)
Because I had to execute the tests many times, and because the majority of the time is consumed by executing the inner tests, I've introduced a new "dummy" makefile target.
It makes it possible to use
  make NSS_TESTS="dummy" NSS_CYCLES="standard"
in order to test that the Android makefile targets work correctly,
without having to waste time with actual NSS test execution.


(e)
I've documented the expected arguments of the sign.sh script
Attachment #641587 - Attachment is obsolete: true
Attachment #649323 - Attachment is obsolete: true
Attachment #641587 - Flags: superreview?(wtc)
Attachment #641587 - Flags: review?(bsmith)
Attachment #687742 - Flags: review?(rrelyea)
Comment on attachment 687742 [details] [diff] [review]
Patch v15

I'll need a slightly tweaked patch in order to make it compatible with our test/tinderbox automation. Final testing underway.
Attachment #687742 - Attachment is obsolete: true
Attachment #687742 - Flags: review?(rrelyea)
Comment on attachment 687742 [details] [diff] [review]
Patch v15

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

This version avoids doing the shsignlib step of the build and thus skips the FIPS tests. I think this is OK. However, I have successfully got the shsignlib step to work by doing the cross-build with a slightly different design. I filed bug 772144 to improve on this.

::: security/coreconf/Linux.mk
@@ +7,5 @@
>  
>  #
>  # The default implementation strategy for Linux is now pthreads
>  #
> +ifndef BUILD_ANDROID

Why not use OS_TARGET=Android to signify this, instead of adding a new macro?

@@ +8,5 @@
>  #
>  # The default implementation strategy for Linux is now pthreads
>  #
> +ifndef BUILD_ANDROID
> +	USE_PTHREADS = 1

Android should be built with USE_PTHREADS. Android's pthreads library is built into -lc, so you just need to avoid adding -lpthread to the linking step on Android.

@@ +23,5 @@
>  DEFAULT_COMPILER = gcc
>  
> +ifdef BUILD_ANDROID
> +ifndef ANDROID_NDK
> +	die "Error: must set ANDROID_NDK to the path to the android NDK first"

Use "$(error ...)" instead of "die ..."

::: security/coreconf/arch.mk
@@ +254,5 @@
> +# this should be  configurable from the user
> +#
> +   OS_TEST := arm
> +   OS_ARCH = Android
> +   ifndef ANDROID_VERSION

Instead of defining a new ANDROID_VERSION symbol, why not use the existing OS_TARGET_RELEASE?

::: security/nss/lib/freebl/unix_rand.c
@@ +362,5 @@
>  
>  static void
>  GiveSystemInfo(void)
>  {
> +#ifndef NO_SYSINFO

Instead of creating a new symbol for NO_SYSINFO, just use #ifndef ANDROID. Otherwise, it is confusing because there are several places where sysinfo is used in this file that aren't guarded by NO_SYSINFO.

::: security/nss/tests/remote/Makefile
@@ +118,5 @@
> +	rm -f $(TESTPACKAGE)
> +	(cd $(DIST)/../.. ; tar czhf dist/$(TESTPACKAGE) runtests.sh dist/$(OBJDIR_NAME) dist/public security/nss/tests security/nss/cmd/bltest/tests security/nss/cmd/shlibsign; echo "created "`pwd`"/dist/$(TESTPACKAGE)" )
> +
> +android_run_tests:
> +	ssh -p $(ANDROID_PORT) -o CheckHostIP=no $(ANDROID_ADDR)  'pwd; cd; pwd; cd nsstest; export PATH=$$HOME/bin:$$PATH ; $(TEST_SHELL) runtests.sh'

I believe that the Android SDK's adb command can do this without us needing SSH.

@@ +121,5 @@
> +android_run_tests:
> +	ssh -p $(ANDROID_PORT) -o CheckHostIP=no $(ANDROID_ADDR)  'pwd; cd; pwd; cd nsstest; export PATH=$$HOME/bin:$$PATH ; $(TEST_SHELL) runtests.sh'
> +
> +android_install:
> +	rm -f $(DIST)/android.sftp

Instead of using sftp, use "adp push". "adp push" is the normal way of pushing files to the phone and it doesn't require installing any additional software on the device.

@@ +132,5 @@
> +
> +WORKDIR="$(DIST)/../../"
> +RESULTSPACKAGE=tests_results.tgz
> +android_get_result:
> +	rm -f $(WORKDIR)/result.sftp $(WORKDIR)/$(RESULTSPACKAGE)

This can be done with adp pull.
Attachment #687742 - Attachment is obsolete: false
Comment on attachment 687742 [details] [diff] [review]
Patch v15

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

Check the splinter review interface for trailing whitepace in a few lines in a few different files.

::: security/nss/Makefile
@@ -150,5 @@
>  package:
>  	$(MAKE) -C pkg publish
>  
> -TESTPACKAGE="nss-$(OS_TARGET)$(CPU_TAG).tgz"
> -package_for_testing:

I don't think we should move package_for_testing from the main build script to under tests/, because package_for_testing depends on several things that aren't under tests/.

::: security/nss/lib/sqlite/config.mk
@@ +37,5 @@
>  endif
> +
> +ifdef BUILD_ANDROID
> +ifndef INTERNAL_TOOLS
> +        DEFINES += -Dfdatasync=fsync

Un-indent?

::: security/nss/tests/remote/Makefile
@@ +55,5 @@
> +#NSS_TESTS="dummy"
> +NSS_CYCLES?="standard pkix upgradedb sharedb"
> +NSS_TESTS?="cipher lowhash libpkix cert dbtests tools sdr crmf smime ssl ocsp merge pkits chains"
> +NSS_SSL_TESTS?="crl normal_normal iopr"
> +NSS_SSL_RUN?="cov auth stress"

I do not think we should set these variables explicitly on Android but not on other platforms, because it is confusing. Are we implicitly disabling some tests on Android? If so, which ones? If we're not disabling any tests here, then why are we treating Android specially?

@@ +68,5 @@
> +	echo "export OS_TARGET=$(OS_TARGET)"   >> $(PCFG)
> +	echo "export DLL_PREFIX=$(DLL_PREFIX)" >> $(PCFG)
> +	echo "export DLL_SUFFIX=$(DLL_SUFFIX)" >> $(PCFG)
> +	echo 'echo "set HOST and DOMSUF if your system is not registered in DNS"' > $(RTSH)
> +	cat $(PCFG)                                  >> $(RTSH)

Why not just send everything about to ">> $(RTSH)" instead of ">> $(PCFG)" and avoid the need for PCFG and this "cat" invocation altogether?

@@ +88,5 @@
> +	echo 'echo "signing"'                               >> $(RTSH)
> +	echo 'cd security/nss/cmd/shlibsign'                >> $(RTSH)
> +	echo '$(TEST_SHELL) ./sign.sh $${DIST}/$${OBJDIR}/ $${DIST}/$${OBJDIR}/bin $${OS_TARGET} $${NSPR_LIB_DIR} $${NSPR_LIB_DIR}$${DLL_PREFIX}freebl3.$${DLL_SUFFIX}'  >> $(RTSH)
> +	echo '$(TEST_SHELL) ./sign.sh $${DIST}/$${OBJDIR}/ $${DIST}/$${OBJDIR}/bin $${OS_TARGET} $${NSPR_LIB_DIR} $${NSPR_LIB_DIR}$${DLL_PREFIX}softokn3.$${DLL_SUFFIX}' >> $(RTSH)
> +	echo '$(TEST_SHELL) ./sign.sh $${DIST}/$${OBJDIR}/ $${DIST}/$${OBJDIR}/bin $${OS_TARGET} $${NSPR_LIB_DIR} $${NSPR_LIB_DIR}$${DLL_PREFIX}nssdbm3.$${DLL_SUFFIX}'  >> $(RTSH)

We should not do the signing of these libraries as part of the testing. It should be done as part of the building of the libraries like it currently is for other platforms. If we do the signing on the test system then that would require our build process to run the tests and extract the signed libraries from the test system, or we'd be testing a different configuration (signed) from the configuration that we ship (unsigned). I think we should just skip these sign.sh invocations and fix the signing properly in the follow-up bug 817633.

@@ +138,5 @@
> +	echo 'get nsstest/$(RESULTSPACKAGE) $(WORKDIR)' >> $(WORKDIR)/result.sftp
> +	sftp -o Port=$(ANDROID_PORT) -o CheckHostIP=no -b $(WORKDIR)/result.sftp $(ANDROID_ADDR) 
> +	(cd $(WORKDIR); tar xzf $(RESULTSPACKAGE); rm -f result.sftp $(RESULTSPACKAGE) )
> +
> +# Android testing assumes having built with: BUILD_ANDROID=1 CROSS_COMPILE=1

It shouldn't be necessary to set CROSS_COMPILE=1 explicitly because BUILD_ANDROID (or, preferably, OS_TARGET=Android) will set CROSS_COMPILE automatically.
I don't have a PC that can be dedicated to the Android NSS testing.

I'm not using a cable, because that's a major headache for me, I want to move my primary work laptop around, without having to care for the Android cable.

Don't ask me to shut off ssh/scp, because it's convenient. I'm doing the work right now, so don't ask me to make my life more difficult. If you think that you have a need for supporting adb push/pull instead, you can implement an incremental patch that adds additional transport targets to the Makefile.
(In reply to Brian Smith (:bsmith) from comment #26)
> 
> This version avoids doing the shsignlib step of the build

No, it executes the signing, as can be seen in the patch, and I've explained.
Look at package_for_testing and the commands to execute sign.sh


> and thus skips the
> FIPS tests. I think this is OK. However, I have successfully got the
> shsignlib step to work by doing the cross-build with a slightly different
> design. 

Maybe you should have shared that earlier, now that I've solved it on my own...


> I filed bug 772144 to improve on this.

You got the bug number wrong, that's this bug.


> > +ifndef BUILD_ANDROID
> 
> Why not use OS_TARGET=Android to signify this, instead of adding a new macro?

I agree that's better.


> > +ifndef BUILD_ANDROID
> > +	USE_PTHREADS = 1
> 
> Android should be built with USE_PTHREADS. Android's pthreads library is
> built into -lc, so you just need to avoid adding -lpthread to the linking
> step on Android.

Can you contribute a patch and test that it works?


> Use "$(error ...)" instead of "die ..."

ok


> > +   ifndef ANDROID_VERSION
> 
> Instead of defining a new ANDROID_VERSION symbol, why not use the existing
> OS_TARGET_RELEASE?

ok


> ::: security/nss/lib/freebl/unix_rand.c
> @@ +362,5 @@
> >  
> >  static void
> >  GiveSystemInfo(void)
> >  {
> > +#ifndef NO_SYSINFO
> 
> Instead of creating a new symbol for NO_SYSINFO, just use #ifndef ANDROID.
> Otherwise, it is confusing because there are several places where sysinfo is
> used in this file that aren't guarded by NO_SYSINFO.

I suspect Bob has deliberately structured it like this, because this is "freebl" code, that often must stay unchanged in Red Hat's maintained branches for a long time. Introducing a specific build time variable gives us greater flexibility to influence the build, rather than coupling it to a specific platform. You should talk to Bob, if you insist on changing.
(In reply to Brian Smith (:bsmith) from comment #27)
> ::: security/nss/Makefile
> @@ -150,5 @@
> >  package:
> >  	$(MAKE) -C pkg publish
> >  
> > -TESTPACKAGE="nss-$(OS_TARGET)$(CPU_TAG).tgz"
> > -package_for_testing:
> 
> I don't think we should move package_for_testing from the main build script
> to under tests/, because package_for_testing depends on several things that
> aren't under tests/.


The "package_for_testing" and the remote makefile targets should stay in the same directory. They are both meant for remote testing. Having them in separate directories requires additional work to setup the make system to jump between directories. I'm not motivated to do that, unless you provide a patch that fixes all issues related to that.

I really don't care where these Makefile targets are located. This was an attempt to address that earlier request to not have them in the main Makefile. If you don't like them to live in a subdirectory, I'm absolutely fine to move them back to the main Makefile.


> ::: security/nss/lib/sqlite/config.mk
> @@ +37,5 @@
> >  endif
> > +
> > +ifdef BUILD_ANDROID
> > +ifndef INTERNAL_TOOLS
> > +        DEFINES += -Dfdatasync=fsync
> 
> Un-indent?

I don't understand.


> ::: security/nss/tests/remote/Makefile
> @@ +55,5 @@
> > +#NSS_TESTS="dummy"
> > +NSS_CYCLES?="standard pkix upgradedb sharedb"
> > +NSS_TESTS?="cipher lowhash libpkix cert dbtests tools sdr crmf smime ssl ocsp merge pkits chains"
> > +NSS_SSL_TESTS?="crl normal_normal iopr"
> > +NSS_SSL_RUN?="cov auth stress"
> 
> I do not think we should set these variables explicitly on Android but not
> on other platforms, because it is confusing.

If it confuses you, we can add a comment.

As you can see, the variables are set only above the Android-specific Makefile targets. They are required for Makefile driven execution of remote testing.

I understand that Bob had selected this subset of tests that he succeeded in testing. 


> Are we implicitly disabling some tests on Android?

Yes. You should read Bob's earlier comments in this bug.


> We should not do the signing of these libraries as part of the testing. It
> should be done as part of the building of the libraries like it currently is
> for other platforms. 

It doesn't matter where we run the signing.
The signing of libs is simply meant as an integrity test.
By signing using the cross-compiled binaries, we don't need a separate build.


> If we do the signing on the test system then that would
> require our build process to run the tests and extract the signed libraries
> from the test system, 

You don't need to do that. In the past I learned that RelEng has setups where the signing is repeated prior to packaging, in order to make sure that everything matches (e.g. because of stripping of symbols from the binary, which requires new signatures).


> It shouldn't be necessary to set CROSS_COMPILE=1 explicitly because
> BUILD_ANDROID (or, preferably, OS_TARGET=Android) will set CROSS_COMPILE
> automatically.

You are making assumptions based on the current state of the world, but who knows, maybe in one year it will be possible to build natively on Android? Then you'll have a headache if you hardcode that conclusion in makefiles.
Your request to use OS_TARGET_RELEASE means that coreconf will look for a file named Android8.mk
Attached patch Patch v21 — — Splinter Review 
Thanks for your hint on IRC regarding TARGET_OSES.

Here is a patch that addresses the proposals that I agreed with.

My local test with this patch is still running, but it looks good so far.
Attachment #687742 - Attachment is obsolete: true
Attachment #687883 - Flags: review?(rrelyea)
Comment on attachment 687883 [details] [diff] [review]
Patch v21

renamed to patch v21
Attachment #687883 - Attachment description: Patch v1 → Patch v21
Depends on: 818062
Target Milestone: --- → 3.14.2
Kai, using this patch, which is based on yours, plus the NSPR patch from bug 817622, I am able to get NSS to build on Windows for Android. I also verified that it continues to build on Linux hosts.

This patch only modifies the build system. It doesn't modify the testing part yet. I haven't had a chance to run the tests on Android.

I made one very significant change in how the build is done. With my patch, you must first build a native host version of NSS before you can cross compile:

# Windows
OS_TARGET=WIN95 make nss_build_all

OS_TARGET=Android \
ANDROID_NDK=/c/mozilla-build/android-ndk-r8c \
NSS_HOST_PREFIX=$PWD/../../dist/WIN954.0_DBG.OBJ make nss_build_all


# Linux
make nss_build_all

OS_TARGET=Android \
NSS_HOST_PREFIX=$PWD/../../dist/Linux3.2_x86_glibc_PTH_DBG.OBJ \
ANDROID_NDK=$HOME/opt/android-ndk-r8c make nss_build_all

This allows us to sign the shared libraries during the build, instead of during the execution of the test.
Attachment #689589 - Flags: feedback?(kaie)
Comment on attachment 689589 [details] [diff] [review]
Additional changes, including changes needed to support building on Windows

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

In this comment, I will note some of the specific changes I made, using the Splinter tool. This comment will be easier to read using the Splinter tool.

::: security/coreconf/Linux.mk
@@ +43,2 @@
>  	ARCHFLAG = --sysroot=$(ANDROID_SYSROOT)
> +	DEFINES += -DANDROID -DRTLD_NOLOAD=0

My patch removes the call to sysinfo() on Linux, and makes the conditional compilation logic for the fork check recognize Android.

RTLD_NOLOAD=0 comes from Gecko's security/build/Makefile.in and without it, I cannot build on Windows. IIRC, the build worked correctly on Linux and this makes me think that the build system is (wrongly) adding some of the host includes to the include path. However, I didn't investigate this.

@@ +147,5 @@
>  # The linker on Red Hat Linux 7.2 and RHEL 2.1 (GNU ld version 2.11.90.0.8)
>  # incorrectly reports undefined references in the libraries we link with, so
>  # we don't use -z defs there.
>  ZDEFS_FLAG		= -Wl,-z,defs
> +DSO_LDOPTS		+= $(ZDEFS_FLAG)

I just realized that I forgot to remove the comment above this line. I do not think we need to do this workaround for those ancient versions of RHEL. Without this change, the Windows-hosted build complains about not finding ld.

::: security/coreconf/Makefile
@@ +6,5 @@
>  CORE_DEPTH	= ..
>  
>  MODULE		= coreconf
>  
> +include $(DEPTH)/coreconf/arch.mk

Needed for $(HOST_OS_ARCH) below.

::: security/coreconf/UNIX.mk
@@ +13,5 @@
>  	OPTIMIZER  += -O
>  	DEFINES    += -UDEBUG -DNDEBUG
>  else
>  	OPTIMIZER  += -g
> +	DEFINES    += -DDEBUG -UNDEBUG

Without these changes to how -DDEBUG_$(USERNAME) is done, the Windows-based build complains because whoami returns "DOMAIN\username", and the backslash causes GCC to (rightly) complain about a bad "#define DOMAIN\username".

Actually, NSPR needs a similar change.

@@ +18,5 @@
>  endif
>  
> +ifeq ($(HOST_OS_ARCH),WINNT)
> +# nsinstall is supplied by MozillaBuild on Windows hosts
> +NSINSTALL      = nsinstall

nsinstall does not build correctly on Windows and also it isn't needed as long as you are using the MozillaBuild environment.

::: security/coreconf/arch.mk
@@ +320,5 @@
>  MK_ARCH = included
> +
> +ifneq ($(HOST_OS_ARCH),$(OS_ARCH))
> +    CROSS_COMPILE = 1
> +endif

This saves us from having to type CROSS_COMPILE=1 on the command line when building, when it is obvious we're cross-compiling.

::: security/nss/cmd/shlibsign/Makefile
@@ +93,2 @@
>  else
> +	LD_LIBRARY_PATH=$(libdir) $(sign_it)

Maybe I need more alternatives, especially for Mac and Solaris, but I don't think we actually need the overly-complex scripts sign.cmd and sign.sh. Even with Kai's helpful comments in his patch, it is still difficult to understand what it is doing.

It would be great to get somebody to test this change on a Mac.

::: security/nss/lib/freebl/Makefile
@@ +176,5 @@
>      DEFINES += -DMP_ASSEMBLY_DIV_2DX1D
>  endif
>  endif # Darwin
>  
> +ifeq (,$(filter-out Android Linux,$(OS_TARGET)))

I did a search for all the references to "Linux" and tried to ensure that, when appropriate, we modify them to add "Android".

Note that this particular change is important for keeping the ARM optimizations in freebl active for Gecko.

@@ +254,5 @@
>  # -Bsymbolic option or the equivalent option for other linkers
>  # to bind the blapi function references in FREEBLVector vector
>  # (ldvector.c) to the blapi functions defined in the freebl
>  # shared libraries.
> +ifeq (,$(filter-out Android BSD_OS FreeBSD Linux NetBSD OpenBSD, $(OS_TARGET)))

I actually do not understand this. Bob should double-check that this makes sense.

@@ +631,5 @@
>  
>  
>  # Bugzilla Bug 333917: the non-x86 code in desblapi.c seems to violate
>  # ANSI C's strict aliasing rules.
> +ifeq (,$(filter-out Android Linux,$(OS_TARGET)))

I read bug 333917 and I don't understand why this fix was targeted to only Linux.

::: security/nss/lib/freebl/unix_rand.c
@@ -362,5 @@
>  
>  static void
>  GiveSystemInfo(void)
>  {
> -#ifndef NO_SYSINFO

No need for NO_SYSINFO because we can just remove the not-so-useful call to sysinfo.
Brian, we had discussed yesterday that your requirements are a new, separate task.

I'm not interested to have this bug made more complicated because of your new requirement to build on Windows.

The patch in this bug makes Linux-hosted Android testing work, and we should commit it first.
Comment on attachment 689589 [details] [diff] [review]
Additional changes, including changes needed to support building on Windows

r- because wrong bug
Attachment #689589 - Flags: feedback?(kaie) → feedback-
(In reply to Kai Engert (:kaie) from comment #37)
> Brian, we had discussed yesterday that your requirements are a new, separate
> task.

No, we said that we will make the support for using "adb push/pull/shell" another bug. I specifically asked in the meeting if we should fold the changes to get the build working on Windows into this bug and we agreed to fold them into this big.

(In reply to Kai Engert (:kaie) from comment #37)
> The patch in this bug makes Linux-hosted Android testing work, and we should
> commit it first.

My patch also fixes several issues with the linux-hosted builds.
(In reply to Brian Smith (:bsmith) from comment #39)
> (In reply to Kai Engert (:kaie) from comment #37)
> > Brian, we had discussed yesterday that your requirements are a new, separate
> > task.
> 
> No, we said that we will make the support for using "adb push/pull/shell"
> another bug. I specifically asked in the meeting if we should fold the
> changes to get the build working on Windows into this bug and we agreed to
> fold them into this big.

That wasn't my understanding.
Kai, if you want to split out the fixes that are applicable to the Linux-hosted builds from my patch, then I will rebase the rest on top of that work. However, I think it will be simpler to just check in the combined patch.
I'm waiting for a review from Bob.

Brian's patch has new changes, and I'm not willing to merge them into my patch, because they aren't required for the initial step and because I haven't tested them.

I'm not willing to take the blame for new side effects for changes that I haven't tested. I want Brian to do that work in a future step that can clearly be blamed to Brian.
Comment on attachment 687883 [details] [diff] [review]
Patch v21

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

::: security/nss/tests/remote/Makefile
@@ +107,5 @@
> +	echo 'cd security/nss/tests'                   >> $(RTSH)
> +	# We require progress indication on stdout while running the tests (to avoid timeouts).
> +	set -o pipefail
> +	echo '$(TEST_SHELL) ./all.sh | tee ../../../logfile 2>&1 |grep ": #"'       >> $(RTSH)
> +	RETVAL=$?

I think you meant to echo this line into the remote test shell script, as well as 'set -o pipefail' 2 lines previous.
Comment on attachment 687883 [details] [diff] [review]
Patch v21

r+ rrelyea
Attachment #687883 - Flags: review?(rrelyea) → review+
Comment on attachment 689589 [details] [diff] [review]
Additional changes, including changes needed to support building on Windows

r- for this patch, to feedback+ for the general idea.

Issues I'd like to see addressed:

Setting USE_PTHREADS have a number of side effects, including adding the _PTH to the OBJECT_DIR. I would like to make sure we aren't messing up the Linux install builds with this change. (I also prefer the shorter name for the android directory if we can keep it).

The change from Tookit 8 to toolkit 5. That is fine for now. Toolkit 8 includes a function that makes the NSPR code which fecthes the library patch actually do something, but the function does not work, so we don't get an advantage.

The shlib sign changes should also handle the case that you have shlbisign on your platform. Either through an environment override or by looking for it in your path.

The rest of the patch looks fine to me.

bob
Attachment #689589 - Flags: review-
(In reply to Robert Relyea from comment #45)
> Comment on attachment 689589 [details] [diff] [review]
> Additional changes, including changes needed to support building on Windows
> 
> r- for this patch, to feedback+ for the general idea.
> 
> Issues I'd like to see addressed:
> 
> Setting USE_PTHREADS have a number of side effects, including adding the
> _PTH to the OBJECT_DIR. I would like to make sure we aren't messing up the
> Linux install builds with this change. (I also prefer the shorter name for
> the android directory if we can keep it).

This is just reverting the change from Kai's patch so that it matches the current code:
https://mxr.mozilla.org/security/source/security/coreconf/Linux.mk#8

Because we allow building with USE_PTHREADS=0, and because Linux has always had "_PTH" in the objdir name, I think it is good to keep the "_PTH" in the objdir name for Android too. (Generally, we should try to avoid non-essential differences in between the Android and Linux build systems, to make our lives easier.)

> The change from Tookit 8 to toolkit 5. That is fine for now. Toolkit 8
> includes a function that makes the NSPR code which fecthes the library patch
> actually do something, but the function does not work, so we don't get an
> advantage.

I don't know what you are referring to here. I am fine with Toolkit 8 being the default but I also think NSPR and NSS should have the same default. Also, Gecko is currently built with Toolkit 5 (but we're switching to 8 soon).

> The shlib sign changes should also handle the case that you have shlbisign
> on your platform. Either through an environment override or by looking for
> it in your path.

I think you can do this already by setting NSS_HOST_PREFIX="/usr" or NSS_HOST_PREFIX="/usr/lib64/nss/unsupported-tools" if shlibsign is installed in one of those directories. (Though, does nsinstall get installed there?)
Comment on attachment 687883 [details] [diff] [review]
Patch v21

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

::: security/nss/lib/sqlite/config.mk
@@ +39,5 @@
> +ifeq ($(OS_TARGET),Android)
> +ifndef INTERNAL_TOOLS
> +        DEFINES += -Dfdatasync=fsync
> +endif
> +endif

This change can be removed now that we've upgrade sqlite.
Comment on attachment 641587 [details] [diff] [review]
Build Android arm using NSS build tools -v 2

>Index: security/nss/tests/cert/cert.sh
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/tests/cert/cert.sh,v
>retrieving revision 1.60
>diff -u -p -r1.60 cert.sh
>--- security/nss/tests/cert/cert.sh	20 Mar 2012 14:47:27 -0000	1.60
>+++ security/nss/tests/cert/cert.sh	12 Jul 2012 20:54:00 -0000
>@@ -1454,7 +1454,9 @@ cert_all_CA
> cert_extended_ssl 
> cert_ssl 
> cert_smime_client        
>-cert_fips
>+if [ -z "NSS_TEST_DISABLE_FIPS" ]; then
>+    cert_fips
>+fi
> cert_eccurves
> cert_extensions
> cert_test_password


The experimental build failed on the non-android platforms.

This test needs a $ for the variable, then it works. I'll add that prior to checkin.
This is an adjusted version of patch v21 as checked in.

It addresses comment comment 47, comment 48, and adds a few additional variables to the remote test script (required for full testing).


Checking in dbm/src/mktemp.c;
/cvsroot/mozilla/dbm/src/mktemp.c,v  <--  mktemp.c
new revision: 3.11; previous revision: 3.10
done
RCS file: /cvsroot/mozilla/security/coreconf/Android.mk,v
done
Checking in security/coreconf/Android.mk;
/cvsroot/mozilla/security/coreconf/Android.mk,v  <--  Android.mk
initial revision: 1.1
done
Checking in security/coreconf/Linux.mk;
/cvsroot/mozilla/security/coreconf/Linux.mk,v  <--  Linux.mk
new revision: 1.53; previous revision: 1.52
done
Checking in security/coreconf/arch.mk;
/cvsroot/mozilla/security/coreconf/arch.mk,v  <--  arch.mk
new revision: 1.25; previous revision: 1.24
done
Checking in security/coreconf/config.mk;
/cvsroot/mozilla/security/coreconf/config.mk,v  <--  config.mk
new revision: 1.34; previous revision: 1.33
done
Checking in security/nss/Makefile;
/cvsroot/mozilla/security/nss/Makefile,v  <--  Makefile
new revision: 1.44; previous revision: 1.43
done
Checking in security/nss/cmd/shlibsign/Makefile;
/cvsroot/mozilla/security/nss/cmd/shlibsign/Makefile,v  <--  Makefile
new revision: 1.22; previous revision: 1.21
done
Checking in security/nss/cmd/shlibsign/sign.sh;
/cvsroot/mozilla/security/nss/cmd/shlibsign/sign.sh,v  <--  sign.sh
new revision: 1.21; previous revision: 1.20
done
Checking in security/nss/lib/freebl/unix_rand.c;
/cvsroot/mozilla/security/nss/lib/freebl/unix_rand.c,v  <--  unix_rand.c
new revision: 1.43; previous revision: 1.42
done
Checking in security/nss/lib/util/secasn1t.h;
/cvsroot/mozilla/security/nss/lib/util/secasn1t.h,v  <--  secasn1t.h
new revision: 1.12; previous revision: 1.11
done
Checking in security/nss/tests/cert/cert.sh;
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.63; previous revision: 1.62
done
Checking in security/nss/tests/chains/chains.sh;
/cvsroot/mozilla/security/nss/tests/chains/chains.sh,v  <--  chains.sh
new revision: 1.40; previous revision: 1.39
done
Checking in security/nss/tests/common/init.sh;
/cvsroot/mozilla/security/nss/tests/common/init.sh,v  <--  init.sh
new revision: 1.75; previous revision: 1.74
done
RCS file: /cvsroot/mozilla/security/nss/tests/dummy/dummy.sh,v
done
Checking in security/nss/tests/dummy/dummy.sh;
/cvsroot/mozilla/security/nss/tests/dummy/dummy.sh,v  <--  dummy.sh
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/tests/remote/Makefile,v
done
Checking in security/nss/tests/remote/Makefile;
/cvsroot/mozilla/security/nss/tests/remote/Makefile,v  <--  Makefile
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/tests/remote/manifest.mn,v
done
Checking in security/nss/tests/remote/manifest.mn;
/cvsroot/mozilla/security/nss/tests/remote/manifest.mn,v  <--  manifest.mn
initial revision: 1.1
done
Marking fixed. Please file a separate bug for the windows-driven android testing.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 11 years ago
Resolution: --- → FIXED
"make" on older platforms doesn't like:

  else ifeq ($(CROSS_COMPILE),1)

Attaching bustage fix.


Checking in Makefile;
/cvsroot/mozilla/security/nss/cmd/shlibsign/Makefile,v  <--  Makefile
new revision: 1.23; previous revision: 1.22
done
Comment on attachment 687883 [details] [diff] [review]
Patch v21

Bob, I think this change broke Windows testing. 

>--- security/nss/tests/chains/chains.sh	19 Oct 2012 20:18:41 -0000	1.38
>+++ security/nss/tests/chains/chains.sh	3 Dec 2012 19:39:30 -0000
>@@ -181,19 +181,17 @@ chains_init()
>     DEFAULT_AIA_BASE_PORT=$(expr ${PORT:-8631} + 10)
>     NSS_AIA_PORT=${NSS_AIA_PORT:-$DEFAULT_AIA_BASE_PORT}
>     NSS_AIA_HTTP=${NSS_AIA_HTTP:-"http://${HOSTADDR}:${NSS_AIA_PORT}"}
>     NSS_AIA_PATH=${NSS_AIA_PATH:-$HOSTDIR/aiahttp}
> 
>     if [ -n "${NSS_AIA_PATH}" ]; then
>         HTTPPID=${NSS_AIA_PATH}/http_pid.$$
>         mkdir -p "${NSS_AIA_PATH}"
>-        pushd "${NSS_AIA_PATH}"
>-        start_httpserv
>-        popd
>+        (cd "${NSS_AIA_PATH}" ; start_httpserv )


You explained the motivation for the change with:

> security/tests/chains/chains.sh - android doesn't have pushd or popd, so just start 
>  httpserve isn a subsell with the correct directory


The new bug is, our tests scripts are no longer able to kill the httpserv process.

This fails, because the function start_httpserv sets an environment variable with the PID that must be killed - because the pidfile approach doesn't work correctly on Windows (I had copied the same start/stop code used for selfserv).

Because that variable is set in a subshell, the parent process never sees it - httpserv stays alive forever - and the test script stalls forever, requiring it to be killed.

    
    

    
  
(In reply to Kai Engert (:kaie) from comment #53)
> Created attachment 699882 [details] [diff] [review]
> windows-testing-fix

Patch confirmed to fix the Windows testing bustage, using a try server build.

Checked in.

Checking in chains.sh;
/cvsroot/mozilla/security/nss/tests/chains/chains.sh,v  <--  chains.sh
new revision: 1.41; previous revision: 1.40
done

    
    

    
  
What environment variable are you setting? I would prefer a solution that still keeps the subshell. something like:

   export HTTPD_PID=`(cd $path ; start _httpserv; echo $HTTPD_PID)`

The downside there is if start_httpserv output somethings to standard out. In which case, perhaps your suggestion would be better. I would prefer some comments in there explaining the start_httpserv side effects, though. shell code that does a cd , does something, then cd back cries out to be turned into a subshell call unless an explanation is given why it shouldn't.

    
    

    
  
But if you look at the many scripts named "runTests.sh" below libpkix/pkix_tests, you can see that code uses the same approach that I've chosen. And they even "source" complete scripts from another directory.

    
    

    
  
libpkix/pkix_tests are sort of a foriegn import. I would expect weird things from java developers;). In any case, as least comment the weirdness when it shows up in main scripts like chains.sh and cert.sh.

Thanks,

bob

    
    

    
  


  
I've checked in this comment.

Checking in chains/chains.sh;
/cvsroot/mozilla/security/nss/tests/chains/chains.sh,v  <--  chains.sh
new revision: 1.42; previous revision: 1.41
done

    
    

    
  
thanks,

bob
No longer blocks: 830046

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: