[rfc][icedtea-web] improvements to reproducers engine

[Danesh Dadachanji]

Hi Jiri,

On 20/05/12 10:18 AM, Jiri Vanek wrote:
> Hi!
>
> During last week I was asked by Danesh and Saad to introduce more hierarchical server and whitelist for reproducers.
> The following patches should add those features. Can you guys (requesters:) mind to review if they match yours needs?
> During the writing i found one bug in current implementation - redundant quotes. I fixed this in third changeset.
> Zipped attachment is example of source and result.
>

Thank you very much for doing this! Greatly appreciated!

> *smallEnchancemtnsToreproducers-jarsInDirs.diff*
>
> I have come many times to issue that I needed jars in more complex directory structure. After speaking with Danesh i come with idea of
> package-like (but reverse) approach.
> If the directory with reproducer will be named something.in.time then in jnlp_testserver will be saved as something/in/time.jar.
> - original something.in.time.jar as created now will remain persisted
> - testcases (definieetly) and resources (I can imagine need to have resource in upper dir) are not affected by this change
> - see the example of source and resutl :)
>
> 2012-05-20  Jiri Vanek <jvanek at redhat.com>
>
>      Reproducers engine enhanced for jars in subdirectories by "." naming convention
>      * Makefile.am: (stamps/change-dots-to-paths.stamp) new target to copy jars
>      with dots (.jar omitted) to the java-like package/directory structure in
>      jnlp_test_server
>      (EXPORTED_TEST_CERT) now depends on  stamps/change-dots-to-paths.stamp
>      (clean-netx-dist-tests) removes stamps/change-dots-to-paths.stamp too.
>

Commenting on the patch file below.

>
> *smallEnchancemtnsToreproducers-whiteList.diff*
>
> This is very simple simplke implementation ow whitelist. It is mentioned for developing purposes.  If developer
> is working on set of reproducer and want to tests them, then he just select them by regexes inside this new file.
> Although it is very simpl echnage I found it very useful.
>
> 2012-05-20  Jiri Vanek <jvanek at redhat.com>
>
>      Introduced whitelist for reproducers
>      * netx-dist-tests-whitelist: new file, contains regular expressions
>      (separated by space) for expr to select testcases which only will be
>      run. By default set to all by expression .*
>      * Makefile.am: (REPRODUCERS_CLASS_NAMES) When class with testcases is
>      going to be included in list, it is at first check for match in whitelist.
>      If there is no match, will not be included.
>
>

Any thoughts on reproducers that depend on other ones' jars? For example, the spaces test and the info/title/vendor tests use 
simpletest1.jar to run. If simpletest1 is not whitelisted then the engine will throw failures when running them. Are we expecting 
testers to know which jars depend on which? If this "dependency" is not going to be checked by the Makefile, then we should definitely 
add a note to the wiki page stating the various dependencies.

>
>
> *smallEnchancemtnsToreproducers-minorFix.diff:*
>
> this ifix is much smaller then it looks like. It actually only removes quotes (which added \n and so wrong interpretation of paths)
>   cp -R --parents "$$notSrcFiles" "$(JNLP_TESTS_DIR)/$$dir/" ; \ ->   cp -R --parents $$notSrcFiles "$(JNLP_TESTS_DIR)/$$dir/" ; \ and
> add test for src directory. The rest is indentation. This changeset should go in in any case.
>
> 2012-05-20  Jiri Vanek <jvanek at redhat.com>
>
>      Fixed error in reproducers source preparation
>      * Makefile.am: (stamps/netx-dist-tests-prepare-reproducers.stamp) removed
>      inappropriately used quotes when copying notSrcFiles. Sucre files now
>      copied only if src dir exists in reproducer.
>

s/Sucre/Source and after this, it looks good to me! Feel free to push this separately.

> smallEnchancemtnsToreproducers-jarsInDirs.diff
>
>
> diff -r 3d01ef4139f4 Makefile.am
> --- a/Makefile.am	Fri May 18 16:23:30 2012 +0200
> +++ b/Makefile.am	Sun May 20 14:36:05 2012 +0200
> @@ -521,8 +524,38 @@
>    	mkdir -p stamps&&  \
>    	touch $@
>
> +	: stamps/netx-dist-tests-sign-some-reproducers.stamp
> +	pushd  $(JNLP_TESTS_SERVER_DEPLOYDIR); \
> +	types=(simple signed); \
> +	for which in "$${types[@]}" ; do \
> +	  . $(abs_top_srcdir)/NEW_LINE_IFS ; \
> +	  simpleReproducers=(`cat $(abs_top_builddir)/junit-jnlp-dist-$$which.txt `); \
> +	  IFS="$$IFS_BACKUP" ; \
> +	  for dir in "$${simpleReproducers[@]}" ; do \

I found this next chunk a bit too complicated. Bash has the power to do all of this for you without expr!

> +	    q=`expr index "$$dir" .`; \
> +	    r=$$? ; \
> +	    if [ $$r = 0  ]; then \
> +	      reverted=`echo $$dir|rev` ; \
> +	      rindex=`expr index $$reverted .` ; \
> +	      length=`expr length $$dir` ; \
> +	      let index=$$length-$$rindex ; \
> +	      let indexplus=$$index+2 ; \
> +	      path=`expr substr $$dir 1 $$index`  ; \
> +	      file=`expr substr $$dir $$indexplus $$rindex`  ; \
> +	      path=`echo $$path | sed "s/\\./\\//g"` ; \
> +	      file="$$file.jar" ; \

Firstly, IMO there should be a check that $$dir does not start or end in a "." and throw an error if there is. I would rather error 
than ignore/remove the padded chars because they should not be there in the first place! Simplest way off the top of my head is

$ test "${dir:0:1}" = "." && $DO_SOMETHING
$ test "${dir:(-1)}" = "." && $DO_SOMETHING

As for the chunk as a whole, I think it would be easier and cleaner to replace it with builtin bash:

+        slashed_dir="./$${dir//.//}" ; \ # Does s/\./\// in variable. Also append "./" to start.
+        path="$${slashed_dir%/*}" ; \ # Mimic dirname
+        file="$${slashed_dir##*/}.jar" ; \ # Mimic basename

Alternatively, you can just use basename and dirname directly too, I just prefer not spawning a subshell for them.

+        slashed_dir="./`echo $dir | sed 's/\\./\\//g'`"
+        path="`dirname $$slashed_dir`" ; \
+        file="`basename $$slashed_dir`.jar" ; \

I think the first method is the cleanest but might not be the easiest to understand (knowing your way around bash strs is not that 
common :/  ) so the second is much simpler to understand but IMO still cleaner than your original method. What do you think? I am fine 
with either FWIW.

These two chunks can replace everything from q=`expr...` up to file="$$file.jar" ; \

More info on string manipulation here[1] (Ctrl+F "${var//old/new}", %/* and ##*/ for the ones I used).

> +	      echo "copying $$dir.jar to $$path as $$file" ; \
> +	      mkdir --parents $$path ; \
> +	      cp $$dir".jar" "$$path"/"$$file" ; \
> +	    fi ; \
> +	  done ; \
> +	done ; \
> +	popd ; \
> +	mkdir -p stamps&&  \
> +	touch $@
> +
>   #this always tries to remove  previous testcert
> -$(EXPORTED_TEST_CERT): stamps/netx-dist-tests-sign-some-reproducers.stamp netx-dist-tests-remove-cert-from-public
> +$(EXPORTED_TEST_CERT): stamps/change-dots-to-paths.stamp netx-dist-tests-remove-cert-from-public
>   	keytool -export -alias $(TEST_CERT_ALIAS) -file $(EXPORTED_TEST_CERT) -storepass $(PRIVATE_KEYSTORE_PASS) -keystore $(PRIVATE_KEYSTORE_NAME)
>
>   stamps/netx-dist-tests-import-cert-to-public: $(EXPORTED_TEST_CERT)
> @@ -827,6 +874,7 @@
>   	rm -f stamps/netx-dist-tests-prepare-reproducers.stamp
>   	rm -f stamps/netx-dist-tests-compile-testcases.stamp
>   	rm -f stamps/netx-dist-tests-sign-some-reproducers.stamp
> +	rm -f stamps/change-dots-to-paths.stamp
>   	rm -f junit-jnlp-dist-simple.txt
>   	rm -f junit-jnlp-dist-signed.txt
>   	rm -f $(REPRODUCERS_CLASS_NAMES)
>

[1] http://mywiki.wooledge.org/BashFAQ/100