[rfc] [icedtea-web] make links

[Jiri Vanek]

On 04/24/2012 05:07 PM, Andrew Hughes wrote:
> Comments on this patch:
>
> 1.  Why the changes to clean-local?  Was there a reason clean-bootstrap-directory
> needed to be earlier?
Omg. This would be regression. The change of order (to make it last) was done with signed applets 
tests. And was necessary. Without it cleaning of applets would fail.
I started to wrote this patch long before signed applets, so this was relict.
Thanx for catch!
> 2.  Is there a reason you're using MOZZILA_{GLOBAL,LOCAL}_BACKUP_FILE instead
> of MOZILLA_{LOCAL,GLOBAL}_BACKUP_FILE?  Thought it was a typo at first.
> 3.  "donot exists" should be "doesn't exist"
> 4.  Why the change to netx-unit-tests-compile?  Like 1, they seem unrelated
> to the rest of the patch.

True. This is unrelated but will be necessary soon. I will post as separate patch when needs come.
>
snip
>
> I don't think the restore ones should depend on the creation ones.
> It seems intuitive, but in my experience, it leads to problems in cleanup.
> They test for existence anyway.
I have removed this dependences. (As tehre is check, then I agree for sure)
>
> For the creation ones, you just need to make sure the plugin exists
> where it's expected to.  I don't think the current targets will do this
> as they just create it in the build directory.
I have added dependenci on $(DESTDIR)$(libdir)/$(BUILT_PLUGIN_LIBRARY). This should do what you want.
>
>>> 2.  You don't check that ${HOME}/.mozilla/plugins exists.
>> It should exists if some mozilla like browser is installed. And I'm
>> checking for installed browsers,
>> so this shouyld be ok.
>
> Only if the browser has been run by the user before.
This does not sounds true... I have tried and this  filesystems always come with browser.
>
>>> 3.  I think using ${HOME} as above may be safer than relying on
>>> tilde expansion by the shell.
>> fixed!
>
> So I see.  I like the use of common variables too.
>
>>> 4.  I don't think we need the messages.  They may have been useful
>>> for debugging but can now be removed.
>> I have removed the messages. But then I have extended it for "unlink"
>>   targets and it become quite
>> confusing. Soi I have added them back and now is clear what are those
>> manual targets doing. I'm for
>> to kept them inside (two levels of conditional branching CAN be
>> confusing).
>
> Looks a lot better.  Fine, other than the typo mentioned above.
typo fixed.
>
>>> 5.  The dependencies on the new targets seem excessive.  All you
>>> need is whatever dependency ensures
>>> the plugin has been installed.  You don't need e.g. javaws.desktop
>>> or docs.stamp because you don't use
>>> them.
>> fied
>
> Point 6...
>
>>> 6.  On that note, surely it should use the plugin from the build
>>> directory as people tend to run "make check"
>>> before "make install"?
>> 50% true, 50% not ;) (If I translated it correctly)
>>
>> make check and make run-netx-dist-tests are independent targets. Make
>> check should be run before
>> make install and have no linking work. But make run-netx-dist-tests
>> MUST have icedtea-wen installed
>> and MUST be run after make install, and is using those new linking
>> targets
>>
>> It give sense to me. (?)
>
> To me, I'd expect them to be part of make check as they're tests.
It can not be. It needs icedtea-web installed.
> I wouldn't expect anything to run after make install.
>
> A build I would expect to go:
>
> 1. configure
> 2. make
> 3. make check (optional)
> 4. make install
   5. make jtreg or test-instaleld-application or ...run-netx-dist-tests

This is really mentioned like testing of final application. netx installed, and plugin linked. It 
then launch javaws and browsers with testcases/reproducers....

jtreg suite is good comparison. it is doing approx. the same. Only our one  does not have gui and is 
integrated inside makefile.

There already was an idea to separate it or to remove this target utterly and to use some existing 
tool (eg jetreg). For now this target (with codecoverage after) is doing its job well.

If there really will come some  big refactoring , then it definitely will come as different changeset.

So I would like to consider this as unrelevant to this patch O:) (If I can be allowed )
>
> with 4 possibly being run by a different user at a completely different
> time (e.g. a user builds it then an admin installs it for them to the system).
> Or 4 could use DESTDIR to do a staged install.
>
> The main issue is your dependencies only create a situation after stage 2.
> It will break if you run make and then one of your test targets.

It will, and I'm hesitating with some restrictions/lightening or more conditional branching. Right 
now, it is practically dependent on make install. This should be enough. On the other side 
run-netx-dist-tests should be dependent on  make global-linkks (xor user-links), or the results of 
tests run will be unpredictable and possibly wrong. But we have agreed that it will not.
Unless I will force this dependence in again, then the only point leading to must of make 
*link/remove*link  will be hint in documentation  - probably in - 
http://icedtea.classpath.org/wiki/Reproducers (with which should be any icedtea-web developer 
familiar anyway;) )


>
> Why would you want to install it before testing it anyway?  I don't understand.
> The user tests could be run as part of make check IMHO, though you may need to
> use xvfb to run the browsers (as the jdk jtreg tests do).

Clarified (2x) above I hope.
>
>>>
snip
>>>
>>

changelog:
2012-04-25  Jiri Vanek<jvanek at redhat.com>

     	Added detection of installed browsers and added targets to create
    	symbolic links from install dir to browsers' plugin directories.
	 Primarily for testing purposes
     	*Makefile.am: (stamps/user-links.stamp) with alias (links) - new target for
     	creating symlinks for all users. One must be root to execute this target.
     	(stamps/global-links.stamp) with alias (user-links) - new target
     	for creating symlinks for logged user only. Because opera is missing this feature,
	 quite useless for testing or dependence targets, but good for live user.
  	(restore-global-links): target for restoring original global links.
  	One must be root again
  	(restore-user-links): target for restoring user's links
     	*configure.ac: added basic check whether and which browsers are
     	installed



Best regards, and thanx for review again ;)

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