[rfc][icedtea-web] Makefile New Whitelisted-Dist-Test option
Jiri Vanek
jvanek at redhat.com
Wed Oct 15 08:51:22 UTC 2014
On 10/10/2014 03:04 PM, Jie Kang wrote:
>
>
> ----- Original Message -----
>> On 09/26/2014 08:16 PM, Jie Kang wrote:
>>>
>>>
>>> ----- Original Message -----
>>>> On 09/23/2014 08:16 PM, Jie Kang wrote:
>>>>> ----- Original Message -----
>>>>>>> On 09/22/2014 11:17 PM, Jie Kang wrote:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> This patch expands on the Makefile Reproducer Test patch [1].
>>>>>>>>>
>>>>>>>>> [1]
>>>>>>>>> http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2014-September/029581.html
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> There is now a new make rule: 'run-netx-whitelisted-dist-tests'
>>>>>>>>>
>>>>>>>>> This rule runs the reproducers similar to 'run-netx-dist-tests',
>>>>>>>>> except it
>>>>>>>>> only processes (compiles, etc.) the resources filtered by the
>>>>>>>>> whitelist.
>>>>>>>>>
>>>>>>>>> As well, the rule 'run-netx-dist-tests' has now reverted back to
>>>>>>>>> processing
>>>>>>>>> all resources (ie. what it used to do before the Makefile Reproducer
>>>>>>>>> Test
>>>>>>>>> patch [1])
>>>>>>>>>
>>>>>>>>> Thoughts?
>>>>>>>>>
>>>>>>>>> Just a note, the purpose of the "stamps/whitelist-filter.stamp" rule
>>>>>>>>> having:
>>>>>>>>> echo ".*" ...
>>>>>>>>> is to maintain compatibility. Though the "all-whitelist" rule also
>>>>>>>>> contains
>>>>>>>>> duplicate code, there can be situations where neither "all-whitelist"
>>>>>>>>> or
>>>>>>>>> "filtered-whitelist" are not called. E.g. if user runs "make
>>>>>>>>> stamps/netx-dist-tests-prepare-reproducers.stamp". This is the
>>>>>>>>> solution I
>>>>>>>>> came up with to allow for all possible make commands to continue to
>>>>>>>>> work
>>>>>>>>> and for the user to be able to quickly switch between running "make
>>>>>>>>> run-netx-dist-tests" and "make run-netx-whitelisted-dist-tests"
>>>>>>>>> without
>>>>>>>>> always having to perform "make clean-netx-dist-tests". I guess a TLDR
>>>>>>>>> is
>>>>>>>>> that it's to prevent regressions.
>>>>>>>>>
>>>>>>>>> If this patch is accepted I will update the wiki and documentation
>>>>>>>>> for
>>>>>>>>> this
>>>>>>>>> feature.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>>
>>>>>>>
>>>>>>> Hm. only hint to approach. Why not configure switch? This seems to me
>>>>>>> overcomplexed. What is
>>>>>>> advantage of this?
>>>>> Hello,
>>>>>
>>>>> I have attached a completely new patch that follows your hint. Thanks a
>>>>> lot, it is much better now, and more simple.
>>>>> New option for configure: --enable-whitelist-processing
>>>>>
>>>>> With the flag on, the Makefile generated will filter by whitelist for
>>>>> processing, otherwise, it will process all, just like before patch [1].
>>>>>
>>>>> [1]http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2014-September/029581.html
>>>>>
>>>>>>> Once the new and old targets will be run in unpredicted order, the
>>>>>>> result
>>>>>>> can
>>>>>>> be unpredictible.
>>>>> The complexity was to make sure running new and old targets in random
>>>>> order
>>>>> still worked hahah. And you could run them and stop them whenever, and
>>>>> still work properly like before too! But now no longer needed so...
>>>>>
>>>>>>> Also I can not see the reverted part in patch.
>>>>> There wasn't really any reversion in code, just reversion in behaviour.
>>>>> Anyways, no longer relevant.
>>>>>
>>>>>
>>>>> Thanks a lot!!
>>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>>>>
>>>>>>> J.
>>>>>>>
>>>>> -- Jie Kang
>>>>>
>>>>>
>>>>> itw-make-configure-whitelist-1.patch
>>>>>
>>>>>
>>>>> diff --git a/Makefile.am b/Makefile.am
>>>>> --- a/Makefile.am
>>>>> +++ b/Makefile.am
>>>>> @@ -178,6 +178,12 @@
>>>>> endif
>>>>> endif
>>>>>
>>>>> +if ENABLE_WHITELIST
>>>>> +WHITELIST=`cat $(REPRODUCERS_CLASS_WHITELIST)`
>>>>> +else
>>>>> +WHITELIST=.*
>>>>> +endif
>>>>> +
>>>>> if WITH_RHINO
>>>>> RHINO_TESTS=stamps/check-pac-functions.stamp
>>>>> else
>>>>> @@ -691,26 +697,23 @@
>>>>> mkdir -p $(REPRODUCERS_BUILD_DIR)
>>>>> touch $@
>>>>>
>>>>> -junit-jnlp-dist-custom.txt: $(REPRODUCERS_CLASS_WHITELIST)
>>>>> +junit-jnlp-dist-custom.txt:
>>>>> cd $(REPRODUCERS_TESTS_SRCDIR)/$(CUSTOM_REPRODUCERS)/ ; \
>>>>> - whiteListed=`cat $(REPRODUCERS_CLASS_WHITELIST)`; \
>>>>> - for x in $$whiteListed ; do \
>>>>> + for x in $(WHITELIST) ; do \
>>>>> find . -maxdepth 1 -mindepth 1 | sed "s/.\/*//" | grep $$x >
>>>>> $(abs_top_builddir)/$@ || true ; \
>>>>> done
>>>>>
>>>>> -junit-jnlp-dist-simple.txt: $(REPRODUCERS_CLASS_WHITELIST)
>>>>> +junit-jnlp-dist-simple.txt:
>>>>> cd $(REPRODUCERS_TESTS_SRCDIR)/simple/ ; \
>>>>> - whiteListed=`cat $(REPRODUCERS_CLASS_WHITELIST)`; \
>>>>> - for x in $$whiteListed ; do \
>>>>> + for x in $(WHITELIST) ; do \
>>>>> find . -maxdepth 1 -mindepth 1 | sed "s/.\/*//" | grep $$x >
>>>>> $(abs_top_builddir)/$@ || true ; \
>>>>> done
>>>>>
>>>>> -stamps/junit-jnlp-dist-signed.stamp: $(REPRODUCERS_CLASS_WHITELIST)
>>>>> +stamps/junit-jnlp-dist-signed.stamp:
>>>>> types=($(SIGNED_REPRODUCERS)) ; \
>>>>> for which in "$${types[@]}" ; do \
>>>>> pushd $(REPRODUCERS_TESTS_SRCDIR)/$$which/ ; \
>>>>> - whiteListed=`cat $(REPRODUCERS_CLASS_WHITELIST)`; \
>>>>> - for x in $$whiteListed ; do \
>>>>> + for x in $(WHITELIST) ; do \
>>>>> find . -maxdepth 1 -mindepth 1 | sed "s/.\/*//" | grep $$x >
>>>>> $(abs_top_builddir)/junit-jnlp-dist-$$which.txt ; \
>>>>> done ; \
>>>>> popd ; \
>>>>> diff --git a/configure.ac b/configure.ac
>>>>> --- a/configure.ac
>>>>> +++ b/configure.ac
>>>>> @@ -28,6 +28,14 @@
>>>>> AM_CONDITIONAL([ENABLE_DOCS], [test x$ENABLE_DOCS = xyes])
>>>>> AC_MSG_RESULT(${ENABLE_DOCS})
>>>>>
>>>>> +AC_MSG_CHECKING([whether to process reproducers using whitelist])
>>>>> +AC_ARG_ENABLE([whitelist-processing],
>>>>> + [AS_HELP_STRING([--enable-whitelist-processing],
>>>>> + [Enable processing of reproducers using whitelist])],
>>>>
>>>> I would probably reflector those sentences. Even to me they are not much
>>>> describing.
>>>> Try to debug those sentences with somebody who never seen itw or our
>>>> testuite :) Once he understood
>>>> the help message, it is right.
>>>>
>>>> for ideas:
>>>> AC_MSG_CHECKING([whether to process reproducers using whitelist])
>>>> - this will be nearly correct if you adapt my idea lower. No it imho
>>>> is
>>>> not, and should be like
>>>> "whether to compile and deploy reproducers using filtering described
>>>> whitelist"
>>>> AC_ARG_ENABLE([whitelist-processing],
>>>> [AS_HELP_STRING([--enable-whitelist-processing],
>>>> [Enable processing of reproducers using whitelist])],
>>>> Same:
>>>> "Enable compilation and deploy of reproducers using filtering described
>>>> whitelist"
>>>>
>>>> In this or in (below) suggested approach, I would like to mention all
>>>> three
>>>> targets (or more?) when
>>>> the whitelist is used - compile, deploy run (some more?)
>>>
>>> Hello,
>>>
>>>
>>> I've altered the messages to explain the purpose better. Does it look okay
>>> to you?
>>>
>>> +AC_MSG_CHECKING([whether to filter by whitelist when processing, compiling
>>> and running reproducers])
>>> +AC_ARG_ENABLE([whitelist-processing],
>>> + [AS_HELP_STRING([--enable-whitelist-processing],
>>> + [Enable whitelist filter when processing, compiling and
>>> running reproducers])],
>>>
>>>
>>>
>>>
>>>
>>>>> + [ENABLE_WHITELIST="${enableval}"], [ENABLE_WHITELIST='no'])
>>>>> +AM_CONDITIONAL([ENABLE_WHITELIST], [test x$ENABLE_WHITELIST = xyes])
>>>>> +AC_MSG_RESULT(${ENABLE_WHITELIST})
>>>>> +
>>>>> AC_PATH_PROG([BIN_BASH], [bash],, [/bin])
>>>>> if test x"$BIN_BASH" = x ; then
>>>>> AC_MSG_ERROR([/bin/bash is used in runtime and for about
>>>>> generation.
>>>>> Dying sooner rather then later])
>>>>>
>>>>
>>>>
>>>> Except this, looks ok.
>>>>
>>>> Well - one general hint.
>>>>
>>>> Right now, if ENABLE_WHITELIST is true, then:
>>>> - only reprodcuers matching regeexes in list are compiled, deployed and
>>>> run
>>>> if ENABLE_WHITELIST is false then:
>>>> - all reprodcuers are compiled and deployed, but only reprodcuers
>>>> matching
>>>> regeexes in whitelist
>>>> are run.
>>>>
>>>>
>>>> I was thinking about to unifying it
>>>> - only reprodcuers matching regeexes in list are compiled, deployed and
>>>> run
>>>> if ENABLE_WHITELIST is false then:
>>>> - all reprodcuers are compiled and deployed, and run
>>>>
>>>
>>> I think this is a good idea as well. The attached patch does this. Can you
>>> check to make sure it looks okay? (changes start at
>>> "-$(REPRODUCERS_CLASS_NAMES): $(REPRODUCERS_CLASS_WHITELIST)")
>>>
>>> Or should this be in another changeset?
>>>
>>>
>>>>
>>>> Well - this have question - what to do with current whitelist. To kept it
>>>> in
>>>> repo enad ampty? As it
>>>> is? To rmeove? I probably incline to first one.
>>>
>>> I think keeping it as is fine. Having the ".*" is good. Making it empty
>>> might make it confusing as to why it is in repo..;
>>>
>>> If we want to remove it I think we'd need a patch to let user supply the
>>> whitelist file location since we can't guarantee it exists with the same
>>> name anymore.;
>>>
>>>
>>>
>>
>> Hi!
>>
>> I have realized an issue with all this filtering.
>> The whitelist is regex on testcase.java files. But for the rest, it seems to
>> be match on directory
>> file. Is there any option, different then extend regex to match dir or
>> testcase?
>>
>> /me hoping tomiss something
>>
>> Imho the correct solution would be
>> a) if tetscase matches, then include all reproducers other files
>> or
>> b)if dir matches include testcase and all other reprodcuer's files
>>
>> I dont like neither :(
>
>
> Hello,
>
> Before reproducers are made by
> 1. Creating list of reproducer directories in text file (junit-jnlp-dist-simple.txt, junit-jnlp-dist-custom.txt, ...)
> 2. Going through list, processing the directories (compile, move resource files, etc.)
> 3. Finally using whitelist on the java files of the processed reproducers to create list of tests to run. And then run what's in the list.
>
> The current HEAD has made it so whitelist is applied to the list of directories in step one. This means that whitelist must be regex on directory and testcase. I think if we choose to whitelist on directory, we don't whitelist on java files and if we choose to whitelist on java files, we don't whitelist on directory. Alternatively, we have two different whitelists... (bad idea).
>
>
> However: Note that this proposed patch, makes it so:
>
> OLD behaviour is back to HEAD.
>
> NEW behaviour is when --enable-whitelist-processing is supplied to ./configure
>
>
> This way, nothing gets changed unless you use: ./configure --enable-whitelist-processing
> I think this is good. Thoughts?
>
>
> Regards,
>
>>
>>
>> J.
>>
>>
>
One more thing to think: My quite often usecase is - lets all reproducers are prepared, and then I'm
Modifying the whitelist, and running always some subset of tests.
I would like not to lsot this usecase:(
As from our IRC converasation - directory x tetscase name issues (+ tests-extensions tests issues)
Any progress?
More thingking about this, more I'm inclined to have two lists:(( One for compilation - based on
directory name (but all testcases compiled), one for run - based on testcase name. But I dont like
two lists. ANy more ideas??
J.
More information about the distro-pkg-dev
mailing list