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

[Jiri Vanek]

On 05/28/2012 09:32 PM, Danesh Dadachanji wrote:
> 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?
Exactly. You can use the "export"ed ones as $(variable) or $$variable. Without export you can not 
use them at all:(
>
>>
>> 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?

removed.. You nitpicker :)

>
>> # 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/.\/*//"`

Yes it is reused. I actually forgot to depend on this in clear-custom-reproducers target. Well and 
it is good habit to generate this lists.

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

done

>
>> + 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. :/

I'm strongly for them. It will be much more buletproof and is causing nearly no effort. I'm 
definitely  against just "make" in directory I would like to have custom targets for sure. And if no 
then definitely forclean. They have to clean after themselves. I consoder it as benefit also for dev 
- you will have "just make" as target for testing. And the necessary stuff will be called by make 
prepare-reproducer. Eg deploy is not necessary for testing oon your own at all. I really would like 
to have special targets for this! IMHO it is making live easier more then complicated.

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

done

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

Fixed as you wish :)

>
>> +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!
Exactly. With effort to reuse as much as possible from simple/signed. Eg certificates.
>
>> +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!

I have tested;) So it IS working. I just did not want to spam with unnecessray reproducer as I was 
aware you will use it soon ;)
Changelog si the same.

Hope taht it helps.

   J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: customMakefiles2.diff
Type: text/x-patch
Size: 11431 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120529/ff7aee14/customMakefiles2.diff