[rfc] [icedtea-web] reproducers with custom makefile

[Danesh Dadachanji]

Hi Jiri,

On 26/05/12 03:39 PM, Jiri Vanek wrote:
> Hi!
>
> We have been speaking with Danes that although current reproducers compilation and signing mechanism is very powerful, sometimes it
> really can not cover some extraordinary case.
> For this purpose we ahve come with idea to have special class of reproducers (next to signed and simple) - in this patch called custom
> - which will have its own makefile and will take care for itself.
> This is implementation of this idea. What do you think? both for idea and implementation :)
>

Thank you very much for doing this! I think it will make the lives of test writers easier for those unique/specific corner cases. I am 
definitely for it!

> One think I do not like on this change -  exporting of variables. In plain Makefile I would use "export". It looks like in Automake
> this is not possible. (I suspect comaptibility issues gnu x normal make), so I have exported most crucial variables manually.
> When I saw result Makefile I was wandering that the makefile is still working.... Well loks like was working correctly. Any hint welcomed!

My autoconf skills are not great to say the least so hopefully someone with more experience can say a few words about this. Just one 
question, by "I have exported most crucial variables manually", you mean export so that they are available to the custom Makefile?

>
> J.
>
>      * Makefile.am:  Most crucial variables exported to be used by custom Makefiles
>      (junit-jnlp-dist-custom.txt): new target scanning for directories in jnlp_tests/custom
>      and saving them as list for future purposes.
>      (stamps/process-custom-reproducers.stamp): new target for iterating by junit-jnlp-dist-custom.txt
>      through  jnlp_tests/custom/* and launching make prepare-reproducer in each
>      (clean-custom-reproducers): same as above but launching make clean-reproducer
>      (run-netx-dist-tests) now depends on stamps/process-custom-reproducers.stamp
>      (clean-netx-dist-tests): now depends on clean-custom-reproducers
>      * tests/jnlp_tests/README: described this mechanism a bit
>
> customMakefiles.diff
>
>
> diff -r 6df151bb5320 Makefile.am
> --- a/Makefile.am	Fri May 25 11:44:13 2012 -0400
> +++ b/Makefile.am	Sat May 26 21:05:53 2012 +0200
> @@ -1,58 +1,58 @@
>   # Source directories
>

[snip]

> +export DTEST_SERVER=-Dtest.server.dir=$(JNLP_TESTS_SERVER_DEPLOYDIR)
> +export DJAVAWS_BUILD=-Djavaws.build.bin=$(DESTDIR)$(bindir)/$(javaws)
> +export DBROWSERS=-Dused.browsers=$(FIREFOX):$(CHROMIUM):$(CHROME):$(OPERA):$(MIDORI):$(EPIPHANY)
> +export REPRODUCERS_DPARAMETERS= $(DTEST_SERVER) $(DJAVAWS_BUILD) $(DBROWSERS)
>   # end of `D`shortcuts
>
> +

Extra newline?

>   # binary names
>   javaws:= $(shell echo javaws | sed '@program_transform_name@')
>   itweb_settings:= $(shell echo itweb-settings | sed '@program_transform_name@')
> @@ -492,6 +493,10 @@
>   	mkdir -p $(JNLP_TESTS_DIR)
>   	touch $@
>
> +junit-jnlp-dist-custom.txt:
> +	cd $(JNLP_TESTS_SRCDIR)/custom/  ; \
> +	find .  -maxdepth 1 -mindepth 1 | sed "s/.\/*//">  $(abs_top_builddir)/$@
> +

Are you adding this as a separate target because this target may be used outside of stamps/process-custom-reproducers.stamp? If not, 
then can't we just merge this directly into the process target? E.g. as

+ customReproducers=`find $(JNLP_TESTS_SRCDIR)/custom/  -maxdepth 1 -mindepth 1 | sed "s/.\/*//"`

>   junit-jnlp-dist-simple.txt:
>   	cd $(JNLP_TESTS_SRCDIR)/simple/  ; \
>   	find .  -maxdepth 1 -mindepth 1 | sed "s/.\/*//">  $(abs_top_builddir)/$@
> @@ -648,7 +653,7 @@
>    javaws.desktop stamps/docs.stamp launcher.build/$(itweb_settings) itweb-settings.desktop \
>    stamps/netx.stamp stamps/junit-jnlp-dist-dirs stamps/netx-dist-tests-import-cert-to-public \
>    stamps/netx-dist-tests-compile.stamp stamps/netx-dist-tests-compile-testcases.stamp $(JUNIT_RUNNER_JAR) \
> - $(TESTS_DIR)/$(REPORT_STYLES_DIRNAME) $(REPRODUCERS_CLASS_NAMES)
> + $(TESTS_DIR)/$(REPORT_STYLES_DIRNAME) $(REPRODUCERS_CLASS_NAMES) stamps/process-custom-reproducers.stamp
>   	cd $(JNLP_TESTS_ENGINE_DIR) ; \
>   	class_names=`cat $(REPRODUCERS_CLASS_NAMES)` ; \
>   	CLASSPATH=$(NETX_DIR)/lib/classes.jar:$(JUNIT_JAR):$(JUNIT_RUNNER_JAR):. \
> @@ -662,6 +667,31 @@
>   endif
>   	touch $@
>
> +stamps/process-custom-reproducers.stamp: stamps/junit-jnlp-dist-dirs stamps/netx-dist-tests-import-cert-to-public \
> + stamps/netx-dist-tests-compile.stamp stamps/netx-dist-tests-compile-testcases.stamp $(JUNIT_RUNNER_JAR) \
> + $(TESTS_DIR)/$(REPORT_STYLES_DIRNAME) $(REPRODUCERS_CLASS_NAMES) junit-jnlp-dist-custom.txt
> +	. $(abs_top_srcdir)/NEW_LINE_IFS ; \
> +	simpleReproducers=(`cat $(abs_top_builddir)/junit-jnlp-dist-custom.txt `); \
> +	IFS="$$IFS_BACKUP" ; \
> +	for dir in "$${simpleReproducers[@]}" ; do \

Please rename simpleReproducers to customReproducers.

> +	  pushd $(JNLP_TESTS_SRCDIR)/custom/$$dir; \
> +	  $(MAKE) prepare-reproducer ; \

Is there any reason for us to force devs to have a prepare-reproducer target? I feel like this is something many will forget, 
particularly the clean-reproducer target as they may not call it ever. :/

> +	  popd ; \
> +	done ;
> +	mkdir -p stamps&&  \
> +	touch $@
> +
> +clean-custom-reproducers:
> +	. $(abs_top_srcdir)/NEW_LINE_IFS ; \
> +	simpleReproducers=(`cat $(abs_top_builddir)/junit-jnlp-dist-custom.txt `); \
> +	IFS="$$IFS_BACKUP" ; \
> +	for dir in "$${simpleReproducers[@]}" ; do \
> +	  pushd $(JNLP_TESTS_SRCDIR)/custom/$$dir; \
> +	  $(MAKE) clean-reproducer ; \
> +	  popd ; \
> +	done ;
> +	rm -f stamps/process-custom-reproducers.stamp
> +

Rename simple to custom here too.

>   #for global-links you must be root, for opera there do not exists user-links
>   #although this targets will indeed create symbolic links to enable
>   #icedtea-web plugin inside browser it is intended for testing purposes
> @@ -992,7 +1022,7 @@
>   	rm -rf  $(TESTS_DIR)/$(REPORT_STYLES_DIRNAME)/
>   	rm -f  $(TESTS_DIR)/index*.html
>
> -clean-netx-dist-tests: clean_tests_reports netx-dist-tests-remove-cert-from-public
> +clean-netx-dist-tests: clean_tests_reports netx-dist-tests-remove-cert-from-public clean-custom-reproducers
>   	rm -f netx-dist-tests-source-files.txt
>   	rm -rf $(JNLP_TESTS_DIR)
>   	rm -rf $(JNLP_TESTS_SERVER_DEPLOYDIR)
> diff -r 6df151bb5320 tests/jnlp_tests/README
> --- a/tests/jnlp_tests/README	Fri May 25 11:44:13 2012 -0400
> +++ b/tests/jnlp_tests/README	Sat May 26 21:05:53 2012 +0200
> @@ -4,7 +4,21 @@
>   Directories are honored in srcs and in resources, but noty in testcases.
>   Directories in signed hande their content in similar way as simle's content is handled,
>    but in addition final jars are signed with simple testkey.
> -Files in advanced directory have to care about themselves, but even those can have some
> - parts inside simple directory, so some parts of them are processed automatically.
> +Files in custom directory have to care about themselves (including testcase),
> + but even those can have some parts inside simple directory, so some parts of them
> + are processed automatically.
>    There are three reproducers – simpletest1, simpletest2 and deadlocktest, which tests
>    test’s suite itself and serve as examples of behaviour.
> +If file in simple or signed directores is composed form dost, then it is placed to
> + directory structure eg my.dir.package will be deployed as
> + jnlp_test_server/my/dir/package.jar (and original kept (eg my.dir.package.jar))

This is a little hard to understand (particularly because this concept is hard to understand :/ ). What do you think of:

If the name of a folder in simple/signed is composed of dots, then its contents are deployed from under a directory structure such that 
each part evaluates to a folder. For example, my.dir.reproducer/ will be deployed as jnlp_test_server/my/dir/reproducer.jar.

> +Inside custom directory are expected directories which are handling and file or
> + directory structure. The only expected file is custom/reproducerName/Makefile. Upon
> + all custom/* are then launched make prepare-reproducer and during cleaning make
> + clean-reproducer Those targets are run after all simple and signed reproducers
> + are prepared, so they can reuse as much code as possible including eg certificates,
> + or testcases directories or resources or dependencies to keep this custom makefiles
> + as simple as possible. Some comment in makefile or readme file is recommended for
> + each custom reproducer to tell dependencies and what it does.


Okay, so correct me if I'm wrong here; Test writers are responsible for building and deploying everything under custom/reproducer/*, 
namely I would have to compile, create the jar, sign it and then copy everything I need into jnlp_test_server/, after which the engine 
will take care of it. If this is correct, then this sounds more than reasonable to me!

> +Because of automake only small set of variables from icedtea-web Makefile is
> + available for custom makefiles, but feel free to export others if needed.
>

I'm going to try to cp some simple reproducer and get it to mimic a deployment as if it were under simple to make sure this patch 
works. Another review coming up! Thanks again for implementing this feature!

Cheers,
Danesh