[rfc] [icedtea-web] make links

[Andrew Hughes]

Comments on this patch:

1.  Why the changes to clean-local?  Was there a reason clean-bootstrap-directory
needed to be earlier?
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.

More below.

> >
> > 2.  You don't provide a way of reverting to the original status
> > quo.
> > I'd suggest:
> >
> > 	    mv -v ${HOME}/.mozilla/plugins/libjavaplugin.so
> > 	    ${HOME}/.mozilla/plugins/libjavaplugin.original.so&&  \
> > 	    ln -s $(DESTDIR)$(libdir)/IcedTeaPlugin.so libjavaplugin.so&&
> > 	     \
> >
> > and adding the inverse to revert back to the original setup after
> > testing.
> >
> >              if [ -e
> >              ${HOME}/.mozilla/plugins/libjavaplugin.original.so ]
> >              ; \
> >                 rm -f ${HOME}/.mozilla/plugins/libjavaplugin.so&&
> >                  \
> >                 mv
> >                 ${HOME}/.mozilla/plugins/libjavaplugin.original.so
> >                 ${HOME}/.mozilla/plugins/libjavaplugin.so ; \
> >              fi ;
> 
> I have added those targets.
> I have copied the the backup to home, because browsers have been
> confused with duplicate entries
> (especially to two links pointing two different versions of same
> plugin ;)

Ok, you'd know better than me.  I don't have any plugins in my browser :-)

> >
> > Other issues:
> >
> > 1.  Why is user-links never called from anywhere?  This is actually
> > less
> > dangerous (doesn't require root) and could be automated.
> Yes. I want to test both linking. One of them had to  be called
> manually.  Now both of them have to be.
> I'm not sure how to handle those linkage dependences and warnings to
> tester.

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.

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.

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

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

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

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.

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

> >
> >> What I'm missing is When I have choose AC_CHECK_PROGS, then it
> >> does
> >> not support --with-val :-/ Is
> >> there any AC_CHECK_PROGS which allows to set with-val?
> >> Becasue I will probably put rather
> >> --with-firefox=/my/preconfiguired/firefox instead of let it use
> >> my default one. Although Lynks is quite good alternative during
> >> tests
> >> runtime :)
> >> Also this can be maybe intorduced as new patch later.
> >>
> >
> > You probably want to look at mirroring something like IT_FIND_JAVAH
> > in IcedTea.
> 
> Yap. Looks close. I would like to have this as separate patch if you
> agree.

Yes, I would suggest the same thing.  Smaller patches focused on a single issue
are a lot easier to review :-)

> >
> >> Best regards J.
> >>
> >> [1]
> >> http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2012-March/017799.html
> >> [2]
> >> http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120320/8ef6bdfe/browserTests-tests-0001.diff
> >> (btw, this page is partially corrupted - there is html test, and
> >> firefox is trying to translate it
> >> as html page :-/.. so see source rather :-/)
> >>
> >>
> >> changelog:
> >> 2012-04-06  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: (clean-local) optionally delets links' stamps
> >> 	(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.
> >> 	*configure.ac: added basic check whether and which browsers are
> >> 	installed
> >>
> >>
> >
> 
> changelog:
> 2012-04-23  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: (clean-local) optionally delets links' stamps
>   	(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
> 	(restore-global-links): target for restoring original global links.
> 	One must be root again
>   user only. Because opera is missing this feature, quite useless.
> 	(restore-user-links): target for rfrstoring user's links
>   	*configure.ac: added basic check whether and which browsers are
>   	installed
> 
> 
> 
> Tahnx for review!
> And best regards!
>       J.
> 

Thanks for doing this work,
-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: 248BDC07 (https://keys.indymedia.org/)
Fingerprint = EC5A 1F5E C0AD 1D15 8F1F  8F91 3B96 A578 248B DC07