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

Wed May 23 04:17:20 PDT 2012

On 05/22/2012 08:58 PM, Danesh Dadachanji wrote:
> 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!

hhmmhmh. I'm still little bit unsure about usefulness (probably only because of lack of ideas 
(except sub dirs (and so lost pacakges in jars))... So I'm glad to hear that appreciated it ;)

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

I had this on mind when I wrote it. Whitelist just filter runs, not compilation and preparation. All 
reproducers (simple, signed and in future also special) are compiled (,signed) , deployed , 
resources copied, testcases compiled. But to the final list for runinning are included just those 
who match the regexes in list

I would really like to avoid some descriptions or checks in reproducers(dependences)! They can make 
the engine to heavyweight.
>
>>
>>
>> *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.

thanx fixed and pushed.

>
>> 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!

Ok. I got inspired and get rid of my nice for-bashdumies-readable code;) And used mixture of your 
examples.
>
>> + 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

Yaaah! Thanx for catch! Included. Dying with error.
>
> 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.

used^

> + 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" ; \

used ^ and ^^.

I preferred those for read-ability.
>
> 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" ; \

They did well. thanx!
>
> 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

Both remaining changelogs reused...

Tahnx for review (not so much for the link :P) !

    J.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smallEnchancemtnsToreproducers-jarsInDirs2.diff
Type: text/x-patch
Size: 2114 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120523/fbc03505/smallEnchancemtnsToreproducers-jarsInDirs2.diff 