[RFC][icedtea-web] - reproducers engine extended for signed reproducers
Omair Majid
omajid at redhat.com
Wed Jun 29 12:12:58 PDT 2011
On 06/26/2011 03:16 PM, Jiri Vanek wrote:
> On 06/24/2011 04:02 PM, Jiri Vanek wrote:
>> based on
>> http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2011-June/014859.html
>> and
>> http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2011-June/014844.html
>>
>>
>> This patch extends reproducers engine for using signed reproducers.
>> All engine is working as before, autotools now ensure that jarsigner
>> and keytool exists in system (if check enabled => test will be lunched)
>>
>> Directory signed is added beside simple. It is handled same way as
>> simple, but jars created from its classes are signed at the end.
>>
>> One demo reproducer included.
>>
>> Everything is quite fine except the fact that javaws is asking for
>> acceptance of cert:) I will handle it asap.
>>
>> Thanx for any comments, regard J.
>
> Demo test is now enchanced for -Xtrustall opinion. Also test ensuring
> nonsigned behaviour being unatached by this opinion is added.
>
Comments included in-line below.
>
> signedReproducersWithXtrustall-noXtrustAllImpl.diff
>
>
> diff -r fc73a2a7f392 Makefile.am
> --- a/Makefile.am Fri Jun 24 11:50:41 2011 +0200
> +++ b/Makefile.am Sun Jun 26 21:05:56 2011 +0200
> @@ -22,6 +22,7 @@
> JNLP_TESTS_ENGINE_DIR=$(TESTS_DIR)/netx/jnlp_testsengine
> JNLP_TESTS_SERVER_DEPLOYDIR=$(TESTS_DIR)/jnlp_test_server
> JNLP_TESTS_DIR=$(TESTS_DIR)/jnlp_tests
> +keystore_name=teststore.ks
>
I think it is more common to use names in CAPITAL_LETTERS.
> JUNIT_RUNNER_JAR=$(abs_top_builddir)/junit-runner.jar
>
> @@ -455,23 +456,45 @@
> @junit-runner-source-files.txt&& \
> $(BOOT_DIR)/bin/jar cf $@ -C $(JUNIT_RUNNER_DIR) .
>
> -junit-jnlp-dist-dirs.txt:
> +junit-jnlp-dist-dirs: junit-jnlp-dist-simple.txt junit-jnlp-dist-signed.txt
> mkdir -p $(JNLP_TESTS_SERVER_DEPLOYDIR)
> mkdir -p $(JNLP_TESTS_DIR)
> mkdir -p $(JNLP_TESTS_ENGINE_DIR)
This target will always get executed.
> +
> +stamps/netx-dist-tests-prepare-reproducers.stamp: junit-jnlp-dist-dirs
> + types=(simple signed); \
> + for which in "$${types[@]}" ; do \
> + simpleReproducers=(`cat $(abs_top_builddir)/junit-jnlp-dist-$$which.txt `); \
> for dir in "$${simpleReproducers[@]}" ; do \
> mkdir -p $(JNLP_TESTS_DIR)/$$dir ; \
> - $(BOOT_DIR)/bin/javac -d $(JNLP_TESTS_DIR)/$$dir/ $(JNLP_TESTS_SRCDIR)/simple/$$dir/srcs/* ; \
> + $(BOOT_DIR)/bin/javac -d $(JNLP_TESTS_DIR)/$$dir/ $(JNLP_TESTS_SRCDIR)/$$which/$$dir/srcs/* ; \
> d=`pwd` ; \
> cd $(JNLP_TESTS_DIR)/$$dir/ ; \
> $(BOOT_DIR)/bin/jar cf $(JNLP_TESTS_SERVER_DEPLOYDIR)/$$dir.jar * ; \
> cd $$d ; \
> - cp -R $(JNLP_TESTS_SRCDIR)/simple/$$dir/resources/* $(JNLP_TESTS_SERVER_DEPLOYDIR)/ ; \
> + cp -R $(JNLP_TESTS_SRCDIR)/$$which/$$dir/resources/* $(JNLP_TESTS_SERVER_DEPLOYDIR)/ ; \
> + done ; \
> + done ; \
> + mkdir -p stamps&& \
> + touch $@
> +
Can you please fix the indentation?
> +stamps/netx-dist-tests-sign-some-reproducers.stamp: stamps/netx-dist-tests-prepare-reproducers.stamp
> + alias=redhat; \
"redhat"? Please use something like icedtea-web instead.
> + keystore=$(abs_top_builddir)/$(keystore_name); \
> + pass=123456789; \
> + $(BOOT_DIR)/bin/keytool -genkey -alias $$alias -keystore $$keystore -keypass $$pass -storepass $$pass -dname "cn=$$alias, ou=$$alias, o=$$alias, c=$$alias" ; \
> + simpleReproducers=(`cat $(abs_top_builddir)/junit-jnlp-dist-signed.txt `); \
> + for dir in "$${simpleReproducers[@]}" ; do \
> + $(JARSIGNER) -keystore $$keystore -storepass $$pass -keypass $$pass $(JNLP_TESTS_SERVER_DEPLOYDIR)/$$dir.jar $$alias ; \
> done ; \
This looks like it will sign jars in-place. The jars are the simple
reproducers. I think that signing the signed reproducers makes more sense.
What happens if JARSIGNER and/or KEYTOOL is not found? Also, shouldn't
you be using $(BOOT_DIR)/bin/jarsigner?
> -stamps/netx-dist-tests-compile-testcases.stamp: stamps/netx.stamp junit-jnlp-dist-dirs.txt\
> +stamps/netx-dist-tests-compile-testcases.stamp: stamps/netx.stamp junit-jnlp-dist-dirs \
> netx-dist-tests-source-files.txt stamps/netx-dist-tests-compile.stamp
> - simpleReproducers=(`cat $(abs_top_builddir)/junit-jnlp-dist-dirs.txt `); \
> + types=(simple signed); \
> + for which in "$${types[@]}" ; do \
> + simpleReproducers=(`cat $(abs_top_builddir)/junit-jnlp-dist-$$which.txt `); \
> for dir in "$${simpleReproducers[@]}" ; do \
> $(BOOT_DIR)/bin/javac $(IT_JAVACFLAGS) \
> -d $(JNLP_TESTS_ENGINE_DIR) \
> -classpath $(JUNIT_JAR):$(NETX_DIR)/lib/classes.jar:$(JNLP_TESTS_ENGINE_DIR) \
> - $(JNLP_TESTS_SRCDIR)/simple/$$dir/testcases/* ; \
> + $(JNLP_TESTS_SRCDIR)/$$which/$$dir/testcases/* ; \
> + done ; \
> done ; \
> mkdir -p stamps&& \
> touch $@
>
Please fix the indentation.
> @@ -623,6 +652,8 @@
> ln -sf $(JAR) $(BOOT_DIR)/bin/jar
> ln -sf $(abs_top_builddir)/javac $(BOOT_DIR)/bin/javac
> ln -sf $(JAVADOC) $(BOOT_DIR)/bin/javadoc
> + ln -sf $(KEYTOOL) $(BOOT_DIR)/bin/keytool
> + ln -sf $(JARSIGNER) $(BOOT_DIR)/bin/jarsigner
> mkdir -p $(BOOT_DIR)/jre/lib&& \
> ln -s $(SYSTEM_JDK_DIR)/jre/lib/rt.jar $(BOOT_DIR)/jre/lib&& \
> if [ -e $(SYSTEM_JDK_DIR)/jre/lib/jsse.jar ] ; then \
> diff -r fc73a2a7f392 acinclude.m4
> --- a/acinclude.m4 Fri Jun 24 11:50:41 2011 +0200
> +++ b/acinclude.m4 Sun Jun 26 21:05:56 2011 +0200
> @@ -617,6 +617,56 @@
> AC_SUBST(JAVA)
> ])
>
> +AC_DEFUN_ONCE([IT_FIND_KEYTOOL],
> +[
> + AC_REQUIRE([IT_CHECK_FOR_JDK])
> + AC_MSG_CHECKING([for keytool])
> + AC_ARG_WITH([keytool],
> + [AS_HELP_STRING(--with-keytool, specify location keytool)],
> + [
> + KEYTOOL="${withval}"
> + ],
> + [
> + KEYTOOL=${SYSTEM_JDK_DIR}/bin/keytool
> + ])
> + if ! test -f "${KEYTOOL}"; then
> + AC_PATH_PROG(KEYTOOL, "${KEYTOOL}")
> + fi
> + if test -z "${KEYTOOL}"; then
> + AC_PATH_PROG(KEYTOOL, "keytool")
> + fi
> + if test -z "${KEYTOOL}"&& test "x$ENABLE_CHECK" = "xyes"; then
> + AC_MSG_ERROR("keytool requred in jdk")
Since you search for keytool programs outside the jdk, perhaps a more
accurate error message might be "keytool not found"?
> + fi
> + AC_MSG_RESULT(${KEYTOOL})
> + AC_SUBST(KEYTOOL)
> +])
> +
> +AC_DEFUN_ONCE([IT_FIND_JARSIGNER],
> +[
> + AC_REQUIRE([IT_CHECK_FOR_JDK])
> + AC_MSG_CHECKING([for jarsigner])
> + AC_ARG_WITH([jarsigner],
> + [AS_HELP_STRING(--with-jarsigner, specify location jarsigner)],
> + [
> + JARSIGNER="${withval}"
> + ],
> + [
> + JARSIGNER=${SYSTEM_JDK_DIR}/bin/jarsigner
> + ])
> + if ! test -f "${JARSIGNER}"; then
> + AC_PATH_PROG(JARSIGNER, "${JARSIGNER}")
> + fi
> + if test -z "${JARSIGNER}"; then
> + AC_PATH_PROG(JARSIGNER, "jarsigner")
> + fi
> + if test -z "${JARSIGNER}"&& test "x$ENABLE_CHECK" = "xyes"; then
> + AC_MSG_ERROR("JARSIGNER requred in jdk")
Same as above. I am also not a fan of copy-pasting like this.
> + fi
> + AC_MSG_RESULT(${JARSIGNER})
> + AC_SUBST(JARSIGNER)
> +])
> +
> AC_DEFUN([IT_FIND_JAVADOC],
> [
> AC_REQUIRE([IT_CHECK_FOR_JDK])
> diff -r fc73a2a7f392 configure.ac
> --- a/configure.ac Fri Jun 24 11:50:41 2011 +0200
> +++ b/configure.ac Sun Jun 26 21:05:56 2011 +0200
> @@ -27,12 +27,23 @@
> AM_CONDITIONAL([ENABLE_DOCS], [test x$ENABLE_DOCS = xyes])
> AC_MSG_RESULT(${ENABLE_DOCS})
>
> +AC_MSG_CHECKING([whether to check])
> +AC_ARG_ENABLE([check],
> + [AS_HELP_STRING([--disable-check],
> + [Disable checking for check specific tools ketool and jarsigner])],
> + [ENABLE_CHECK="${enableval}"], [ENABLE_CHECK='yes'])
> +AM_CONDITIONAL([ENABLE_CHECK], [test x$ENABLE_CHECK = xyes])
> +AC_MSG_RESULT(${ENABLE_CHECK})
> +
This seems rather odd. What does a tool have to do to be included in
'specific tools'? I think it might be better to check if the user
specified --with-jarsigner=no. If the user did, skip checking for
jarsigner, signing the actual jars and the signed tests. Of course, you
can change things so that jarsigner/keytool are mandatory. That's fine
with me too.
> + public ReadPropertiesSignedTest() {
> + List<String> ll=new ArrayList<String>(1);
> + ll.add("-Xtrustall");
> + l=Collections.unmodifiableList(ll);
> + }
> +
The indentation seems a little strange.
Cheers,
Omair
More information about the distro-pkg-dev
mailing list