« Return to Thread: [rfc] [icedtea-web] make links

Re: [rfc] [icedtea-web] make links

by Jiri Vanek :: Rate this Message:

| View in Thread

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

      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.

[makeLinks3.diff]

diff -r 82e908d46d70 Makefile.am
--- a/Makefile.am Tue Apr 24 14:43:34 2012 -0400
+++ b/Makefile.am Wed Apr 25 12:08:18 2012 +0200
@@ -26,7 +26,7 @@
 PRIVATE_KEYSTORE_PASS=123456789
 EXPORTED_TEST_CERT=icedteatests.crt
 TEST_CERT_ALIAS=icedteaweb
-PUBLIC_KEYSTORE=~/.icedtea/security/trusted.certs
+PUBLIC_KEYSTORE=${HOME}/.icedtea/security/trusted.certs
 PUBLIC_KEYSTORE_PASS=changeit
 
 JUNIT_RUNNER_JAR=$(abs_top_builddir)/junit-runner.jar
@@ -35,6 +35,19 @@
 EMMA_JAVA_ARGS=-Xmx2G
 META_MANIFEST = META-INF/MANIFEST.MF
 
+# linking variables
+PLUGIN_LINK_NAME=libjavaplugin.so
+MOZILLA_LOCAL_PLUGINDIR=${HOME}/.mozilla/plugins
+MOZILLA_GLOBAL64_PLUGINDIR=/usr/lib64/mozilla/plugins
+MOZILLA_GLOBAL32_PLUGINDIR=/usr/lib/mozilla/plugins
+OPERA_GLOBAL64_PLUGINDIR=/usr/lib64/opera/plugins
+OPERA_GLOBAL32_PLUGINDIR=/usr/lib/opera/plugins
+BUILT_PLUGIN_LIBRARY=IcedTeaPlugin.so
+MOZILLA_LOCAL_BACKUP_FILE=${HOME}/$(PLUGIN_LINK_NAME).origU
+MOZILLA_GLOBAL_BACKUP_FILE=${HOME}/$(PLUGIN_LINK_NAME).origMG
+OPERA_GLOBAL_BACKUP_FILE=${HOME}/$(PLUGIN_LINK_NAME).origOG
+# end of linking variables
+
 # Build directories
 
 BOOT_DIR = $(abs_top_builddir)/bootstrap/jdk1.6.0
@@ -86,7 +99,7 @@
 PLUGIN_DIR=$(abs_top_builddir)/plugin/icedteanp
 PLUGIN_SRCDIR=$(abs_top_srcdir)/plugin/icedteanp
 LIVECONNECT_SRCS = $(PLUGIN_SRCDIR)/java
-ICEDTEAPLUGIN_TARGET = $(PLUGIN_DIR)/IcedTeaPlugin.so stamps/liveconnect-dist.stamp
+ICEDTEAPLUGIN_TARGET = $(PLUGIN_DIR)/$(BUILT_PLUGIN_LIBRARY) stamps/liveconnect-dist.stamp
 PLUGIN_PKGS = sun.applet netscape.security netscape.javascript
 endif
 
@@ -161,7 +174,7 @@
 install-exec-local:
  ${mkinstalldirs} $(DESTDIR)$(bindir) $(DESTDIR)$(datadir)/$(PACKAGE_NAME)/ $(DESTDIR)$(libdir)
 if ENABLE_PLUGIN
- ${INSTALL_PROGRAM} $(PLUGIN_DIR)/IcedTeaPlugin.so $(DESTDIR)$(libdir)
+ ${INSTALL_PROGRAM} $(PLUGIN_DIR)/$(BUILT_PLUGIN_LIBRARY) $(DESTDIR)$(libdir)
  ${INSTALL_DATA} $(abs_top_builddir)/liveconnect/lib/classes.jar $(DESTDIR)$(datadir)/$(PACKAGE_NAME)/plugin.jar
 endif
  ${INSTALL_DATA} $(NETX_DIR)/lib/classes.jar $(DESTDIR)$(datadir)/$(PACKAGE_NAME)/netx.jar
@@ -190,7 +203,7 @@
 endif
 
 uninstall-local:
- rm -f $(DESTDIR)$(libdir)/IcedTeaPlugin.so
+ rm -f $(DESTDIR)$(libdir)/$(BUILT_PLUGIN_LIBRARY)
  rm -f $(DESTDIR)$(datadir)/$(PACKAGE_NAME)/plugin.jar
  rm -f $(DESTDIR)$(datadir)/$(PACKAGE_NAME)/netx.jar
  rm -f $(DESTDIR)$(datadir)/$(PACKAGE_NAME)/about.jnlp
@@ -233,7 +246,7 @@
   $(MOZILLA_CFLAGS) \
   -fPIC -o $@ -c $<
 
-$(PLUGIN_DIR)/IcedTeaPlugin.so: $(addprefix $(PLUGIN_DIR)/,$(PLUGIN_OBJECTS))
+$(PLUGIN_DIR)/$(BUILT_PLUGIN_LIBRARY): $(addprefix $(PLUGIN_DIR)/,$(PLUGIN_OBJECTS))
  cd $(PLUGIN_DIR) && \
  $(CXX) $(CXXFLAGS) \
   $(PLUGIN_OBJECTS) \
@@ -244,7 +257,7 @@
 
 clean-IcedTeaPlugin:
  rm -f $(PLUGIN_DIR)/*.o
- rm -f $(PLUGIN_DIR)/IcedTeaPlugin.so
+ rm -f $(PLUGIN_DIR)/$(BUILT_PLUGIN_LIBRARY)
  if [ $(abs_top_srcdir) != $(abs_top_builddir) ]; then \
   if [ -e $(abs_top_builddir)/plugin/icedteanp ] ; then \
     rmdir $(abs_top_builddir)/plugin/icedteanp ; \
@@ -582,7 +595,7 @@
  cd $(JNLP_TESTS_ENGINE_DIR) ; \
  class_names=`cat $(REPRODUCERS_CLASS_NAMES)` ; \
  CLASSPATH=$(NETX_DIR)/lib/classes.jar:$(JUNIT_JAR):$(JUNIT_RUNNER_JAR):. \
-  $(BOOT_DIR)/bin/java -Dtest.server.dir=$(JNLP_TESTS_SERVER_DEPLOYDIR) -Djavaws.build.bin=$(DESTDIR)$(bindir)/$(javaws) \
+  $(BOOT_DIR)/bin/java -Dtest.server.dir=$(JNLP_TESTS_SERVER_DEPLOYDIR) -Djavaws.build.bin=$(DESTDIR)$(bindir)/$(javaws) -Dused.browsers=$(FIREFOX):$(CHROMIUM):$(CHROME):$(OPERA) \
  -Xbootclasspath:$(RUNTIME) CommandLine $$class_names \
   > stdout.log 2> stderr.log ; \
  cat stdout.log ; \
@@ -592,6 +605,112 @@
 endif
  touch $@
 
+#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
+if ENABLE_PLUGIN
+stamps/user-links.stamp: stamps/netx-dist.stamp extra-lib/about.jar stamps/plugin.stamp \
+ launcher.build/$(javaws) stamps/netx.stamp $(DESTDIR)$(libdir)/$(BUILT_PLUGIN_LIBRARY)
+ if [ "$(FIREFOX)" != "" -o "$(CHROMIUM)" != "" -o "$(CHROME)" != "" ]  ; then  \
+  if [ -e $(MOZILLA_LOCAL_PLUGINDIR)/$(PLUGIN_LINK_NAME) ] ; then \
+    mv -f $(MOZILLA_LOCAL_PLUGINDIR)/$(PLUGIN_LINK_NAME)  $(MOZILLA_LOCAL_BACKUP_FILE) ; \
+    echo "$(MOZILLA_LOCAL_PLUGINDIR)/$(PLUGIN_LINK_NAME) backuped as $(MOZILLA_LOCAL_BACKUP_FILE)" ; \
+  else \
+    echo "$(MOZILLA_LOCAL_PLUGINDIR)/$(PLUGIN_LINK_NAME) doesn't exists, nothing to be backuped to $(MOZILLA_LOCAL_BACKUP_FILE)" ; \
+  fi ; \
+  pushd $(MOZILLA_LOCAL_PLUGINDIR) ; \
+  ln -s $(DESTDIR)$(libdir)/$(BUILT_PLUGIN_LIBRARY) $(PLUGIN_LINK_NAME) ; \
+  echo "$(DESTDIR)$(libdir)/$(BUILT_PLUGIN_LIBRARY) linked as  $$PWD/$(PLUGIN_LINK_NAME)" ; \
+  popd ; \
+ fi ; \
+ touch $@
+
+restore-user-links:
+ if [ "$(FIREFOX)" != "" -o "$(CHROMIUM)" != "" -o "$(CHROME)" != "" ]  ; then  \
+  if [ -e $(MOZILLA_LOCAL_BACKUP_FILE) ] ; then \
+    mv -f  $(MOZILLA_LOCAL_BACKUP_FILE)  $(MOZILLA_LOCAL_PLUGINDIR)/$(PLUGIN_LINK_NAME) ; \
+    echo "$(MOZILLA_LOCAL_BACKUP_FILE) restored as $(MOZILLA_LOCAL_PLUGINDIR)/$(PLUGIN_LINK_NAME)" ; \
+  else \
+    rm -f $(MOZILLA_LOCAL_PLUGINDIR)/$(PLUGIN_LINK_NAME) ; \
+    echo "$(MOZILLA_LOCAL_BACKUP_FILE) do not exists, nothing to be restored. $(MOZILLA_LOCAL_PLUGINDIR)/$(PLUGIN_LINK_NAME) removed" ; \
+  fi ; \
+ fi ;
+ if [ -e stamps/user-links.stamp ] ; then \
+  rm -f stamps/user-links.stamp ; \
+ fi
+
+stamps/global-links.stamp: stamps/netx-dist.stamp extra-lib/about.jar stamps/plugin.stamp launcher.build/$(javaws) \
+ stamps/netx.stamp $(DESTDIR)$(libdir)/$(BUILT_PLUGIN_LIBRARY)
+ if [ "$(FIREFOX)" != "" -o "$(CHROMIUM)" != "" -o "$(CHROME)" != "" ]  ; then  \
+    dir="$(MOZILLA_GLOBAL32_PLUGINDIR)"  ; \
+    arch=`arch`  ; \
+    if [ "$$arch" = "x86_64" ]  ; then \
+      dir="$(MOZILLA_GLOBAL64_PLUGINDIR)"  ; \
+    fi ; \
+    if [ -e "$$dir"/$(PLUGIN_LINK_NAME) ] ; then \
+      mv -f "$$dir"/$(PLUGIN_LINK_NAME)  $(MOZILLA_GLOBAL_BACKUP_FILE) ; \
+      echo "$$dir/$(PLUGIN_LINK_NAME) backuped as  $(MOZILLA_GLOBAL_BACKUP_FILE)" ; \
+    else \
+      echo "$$dir/$(PLUGIN_LINK_NAME) do not exists, nothing to be backuped to $(MOZILLA_GLOBAL_BACKUP_FILE)" ; \
+    fi ; \
+    pushd "$$dir"  ; \
+    ln -s $(DESTDIR)$(libdir)/$(BUILT_PLUGIN_LIBRARY) $(PLUGIN_LINK_NAME)  ; \
+    echo "$(DESTDIR)$(libdir)/$(BUILT_PLUGIN_LIBRARY) linked as  $$PWD/$(PLUGIN_LINK_NAME)" ; \
+    popd  ; \
+ fi ;
+ if [ "$(OPERA)" != "" ]  ; then \
+    dir="$(OPERA_GLOBAL32_PLUGINDIR)"  ; \
+    arch=`arch`  ; \
+    if [ "$$arch" = "x86_64" ] ; then \
+      dir="$(OPERA_GLOBAL64_PLUGINDIR)" ; \
+    fi ; \
+    if [ -e "$$dir"/$(PLUGIN_LINK_NAME) ] ; then \
+      mv -f "$$dir"/$(PLUGIN_LINK_NAME)  $(OPERA_GLOBAL_BACKUP_FILE) ; \
+      echo "$$dir/$(PLUGIN_LINK_NAME) backuped as  $(OPERA_GLOBAL_BACKUP_FILE) "; \
+    else \
+      echo "$$dir/$(PLUGIN_LINK_NAME) do not exists, nothing to be backuped to $(OPERA_GLOBAL_BACKUP_FILE) "; \
+    fi ; \
+    pushd "$$dir"  ; \
+    ln -s $(DESTDIR)$(libdir)/$(BUILT_PLUGIN_LIBRARY) $(PLUGIN_LINK_NAME)  ; \
+    echo "$(DESTDIR)$(libdir)/$(BUILT_PLUGIN_LIBRARY) linked as  $$PWD/$(PLUGIN_LINK_NAME)" ; \
+    popd  ; \
+ fi   ; \
+ touch $@
+
+restore-global-links:
+ if [ "$(FIREFOX)" != "" -o "$(CHROMIUM)" != "" -o "$(CHROME)" != "" ]  ; then  \
+    dir="$(MOZILLA_GLOBAL32_PLUGINDIR)"  ; \
+    arch=`arch`  ; \
+    if [ "$$arch" = "x86_64" ]  ; then \
+      dir="$(MOZILLA_GLOBAL64_PLUGINDIR)"  ; \
+    fi ; \
+    if [ -e $(MOZILLA_GLOBAL_BACKUP_FILE) ] ; then \
+      mv -f $(MOZILLA_GLOBAL_BACKUP_FILE) "$$dir"/$(PLUGIN_LINK_NAME) ; \
+      echo "$(MOZILLA_GLOBAL_BACKUP_FILE) restored as $$dir/$(PLUGIN_LINK_NAME)" ; \
+    else \
+      rm -f "$$dir"/$(PLUGIN_LINK_NAME) ; \
+      echo "$(MOZILLA_GLOBAL_BACKUP_FILE) do not exists, nothing to be restored. $$dir/$(PLUGIN_LINK_NAME) removed" ; \
+    fi ; \
+ fi ;
+ if [ "$(OPERA)" != "" ]  ; then \
+    dir="$(OPERA_GLOBAL32_PLUGINDIR)"  ; \
+    arch=`arch`  ; \
+    if [ "$$arch" = "x86_64" ] ; then \
+      dir="$(OPERA_GLOBAL64_PLUGINDIR)" ; \
+    fi ; \
+    if [ -e $(OPERA_GLOBAL_BACKUP_FILE) ] ; then \
+      mv -f $(OPERA_GLOBAL_BACKUP_FILE) "$$dir"/$(PLUGIN_LINK_NAME) ; \
+      echo "$(OPERA_GLOBAL_BACKUP_FILE)  restored as $$dir/$(PLUGIN_LINK_NAME)" ; \
+    else \
+      rm -f "$$dir"/$(PLUGIN_LINK_NAME) ; \
+      echo "$(OPERA_GLOBAL_BACKUP_FILE) do not exist, nothing to be restored. $$dir/$(PLUGIN_LINK_NAME) removed" ; \
+    fi ; \
+ fi   ;
+ if [ -e stamps/global-links.stamp ] ; then \
+  rm -f stamps/global-links.stamp ; \
+ fi
+endif
+
 netx-unit-tests-source-files.txt:
  find $(NETX_UNIT_TEST_SRCDIR) -name '*.java' | sort > $@
 
@@ -969,6 +1088,10 @@
 
 run-netx-unit-tests: stamps/run-netx-unit-tests.stamp
 
+links: stamps/global-links.stamp
+
+user-links: stamps/user-links.stamp
+
 run-netx-dist-tests: stamps/run-netx-dist-tests.stamp
 
 run-unit-test-code-coverage: stamps/run-unit-test-code-coverage.stamp
diff -r 82e908d46d70 configure.ac
--- a/configure.ac Tue Apr 24 14:43:34 2012 -0400
+++ b/configure.ac Wed Apr 25 12:08:18 2012 +0200
@@ -88,6 +88,11 @@
 #
 
 AC_CHECK_PROGS([XSLTPROC],[xsltproc],[], [])
+AC_CHECK_PROGS([FIREFOX],[firefox],[], [])
+AC_CHECK_PROGS([CHROME],[google-chrome],[], [])
+AC_CHECK_PROGS([CHROMIUM],[chromium-browser],[], [])
+AC_CHECK_PROGS([OPERA],[opera],[], [])
+
 AM_CONDITIONAL([WITH_XSLTPROC], [test x"$XSLTPROC" != x ])
 IT_FIND_OPTIONAL_JAR([rhino], RHINO,
     [/usr/share/java/js.jar /usr/share/rhino-1.6/lib/js.jar])

 « Return to Thread: [rfc] [icedtea-web] make links