Bug 24357

In fact on Linux at least it does seem to work for updates: the old
permissions are kept.

Leaving open for the case of new modules.

Target milestone was changed from '3.4' to TBD.

reassigne to Hrebejk, new owner of autupdate 

reassigne to Jirka - new owner of autoupdate

It's more likely a task on building NBMs to use smarter zip utility to hold file permission in NBM archive. I guess then
Plugin Manager could take care to hold as same permission on unpacked files from archive.

*** Issue 150524 has been marked as a duplicate of this issue. ***

Quoted from mail discussion:
Jesse Glick wrote:
> JDK 6 has direct support for querying and setting basic file permission 
> bits in java.io.File. In JDK 5, this can be done only be forking a Unix 
> executable or using e.g. JNA.
> 
> Ant's <zip> implementation (written from scratch) is capable of storing 
> file permissions upon request. AFAIK it makes no attempt to pick up such 
> permissions automatically from files it is actually packing (and this 
> would anyway be rather difficult prior to JDK 6); rather, you can 
> specify a file mode in the build script for certain ZIP entries.
> 
> NB's <makenbm> uses Ant's ZIP implementation internally, so could be 
> modified somehow to include file mode bits based on some information.
> 
> AU's updater.jar uses java.util.zip, which has no representation of file 
> mode bits. If you wanted to set +x when unpacking NBMs, you would need 
> to somehow identify which files required this bit. Possibilities 
> include: (a) using a different ZIP implementation which can report on 
> the bits in the ZIP file; (b) requiring such files to be enumerated 
> somewhere in Info.xml or elsewhere in the NBM; (c) detecting likely 
> executables, e.g. any *.sh, or any file starting with "#!/", or any file 
> starting with an ELF magic header, etc Still pretty easy in JDK 5:
> just Runtime.exec /bin/chmod.
> 
> NBMs are packed from the cluster directory; default build targets in 
> common.xml etc. never set executable bits on files copied to this dir, 
> but build.xml overrides can be made to do so for specific cases.

Reassigning to the new "autoupdate/*" owner dlipin.

The proposed fix for option (c) is attached.
It does the following for all extracted files :
1) run /usr/bin/file and checkes whether the file is reported as "executable", "shared object", "shared library" or 
"dynamic lib"
2) checks if the file has the ELF magic header or the interpreter string (#!/)

At the end, it executes either /bin/chmod or /usr/bin/chmod on all the files at once (if running JDK5) or invokes 
java.io.File.setExecutable(true, false) if it is available (JDK6 and later).

Created attachment 78032 [details]
patch that sets the file executable bits on Unix in ModuleUpdater

Out of curiosity: Should not your write a unit test to verify that the intended behaviour works forever?

likely yes, but unfortunately there is no tests for the o.n.updater at all (except simple test on UpdaterDispatcher).
There is likely a reason for such situation but I don`t know it.

I guess that it can be tested via tests in o.n.m.autoupdate.services, I`ll write a new one soon.

Almost every test for autoupdate.services needs to deal with updater. Just modify or add a bit of code to one of the 
existing and test that permissions are OK. Imho.

Attaching the fix and test for it.

Created attachment 78060 [details]
ModuleUpdater fix, org-yourorghere-executable-permissions.nbm and ExecutablePermissionsTest.java

Does it work on Mac OS X as well as Linux and Solaris? Is /usr/bin/file universally available and does it have similar
output?

BTW varargs:

  File.class.getMethod("setExecutable", new Class[] {Boolean.class, Boolean.class})

can be simplified to

  File.class.getMethod("setExecutable", Boolean.class, Boolean.class)

Anyway I expect that you meant the primitive Boolean.TYPE, not Boolean.class which represents the wrapper class.

> Does it work on Mac OS X as well as Linux and Solaris? 
> Is /usr/bin/file universally available and does it have similar output?
I`ve checked Linux (FC4, Ubuntu 8.10), Solaris 10 (sparc,x86), AIX 5.3, Mac OS X 10.4 and /usr/bin/file is available on 
all of them. I have also OpenBSD and FreeBSD - I`ll check them after unzip the old VBox images that I`ve created 
recently. Unfortunately I have no access to HP-UX. And the rest of systems is almost nothing among NB users.

Yes, the output pretty much the same. It is the one-line output with colon as the separator between the file path and 
file "type". Certainly, the number of supported files that /usr/bin/file can correctly identify depends on the system.

The only exception is the Mac OS X with its universal binaries. In that case the output of file is 4 lines long - but 
it can also be identified by "universal binary" substring.
$ file  /Applications/Chess.app/Contents/MacOS/Chess
/Applications/Chess.app/Contents/MacOS/Chess: Mach-O universal binary with 4 architectures
/Applications/Chess.app/Contents/MacOS/Chess (for architecture ppc):    Mach-O executable ppc
/Applications/Chess.app/Contents/MacOS/Chess (for architecture ppc64):  Mach-O 64-bit executable ppc64
/Applications/Chess.app/Contents/MacOS/Chess (for architecture i386):   Mach-O executable i386
/Applications/Chess.app/Contents/MacOS/Chess (for architecture x86_64): Mach-O 64-bit executable x86_64


> BTW varargs:
> 
>  File.class.getMethod("setExecutable", new Class[] {Boolean.class, Boolean.class})
>
> can be simplified to
>
>  File.class.getMethod("setExecutable", Boolean.class, Boolean.class)
> 
> Anyway I expect that you meant the primitive Boolean.TYPE, not Boolean.class which represents the wrapper class.
Sure, thanks, Boolean.TYPE is the right thing.

Created attachment 78070 [details]
Using Boolean.TYPE in setExecutable method retrieval, identify "universal binary" and "shell script" as the executables

I can also try to play with option (b) in the following way:
1) introduce property e.g. "executable.files" in nbproject/project.properties
2) during making NBM (MakeNBM) read that "executable.files" property and process it as the <includes> ant type i.e. 
masks are supported. All file paths store in Info/executables.list.
3) During building of the NB plugin project and copying all the files to nbbuild/netbeans (who does that? 
apisupport.harness? which target? "release"?) also read that "executable.files" property and after that do the chmod - 
it will help to avoid doing <chmod> in build.xml in the particular modules (e.g. cnd/build.xml, o.jruby.distro/
build.xml and such).
4) During NBM installation read that Info/executable.list and fix the permissions.

Attaching the other try to fix this issue according to 4-steps changes listed in the previous comment.
This diff makes the point (4).

Created attachment 78143 [details]
ModuleUpdater reads Info/executable.list and chmods the listed files appropriately

Anyone interested please review the patch, it touches autoupdate.services, apisupport.harness, nbbuild. I`m going to 
integrate it early next week.

The solution is described in my comment from "Wed Mar 11 18:58:34 +0000 2009" with the only correction in the property 
name (nbm.executable.files).

Created attachment 85957 [details]
patch to enable setting executable permissions on module build and nbm installation

Created attachment 85959 [details]
hg bundle file with base rev#c4c83e0801de

[JG01] nbm.executable.files=launchers/app.sh in apisupport.harness is unnecessary AFAIK; this file is not run in place,
it is packed into a ZIP by an Ant target (during which time it is a+x).


[JG02] Prefer comma-separated properties to space-separated properties. In this case probably either separator is
accepted, but documentation should recommend comma-separated.


[JG03] If you change taskdefs.properties you probably must also change bundled.tasks in project.properties. Test on an
external standalone module to check.


[JG04] I think BufferedReader would be a lot simpler and clearer in readExecutableFilesList. Not to mention that your
impl ignores encoding. Use UTF-8. Don't use String.trim() without a compelling reason.


[JG05] java.version is the runtime environment version. Probably you meant java.specification.version. Anyway it is
probably more straightforward to check for File.setExecutable, catching NoSuchMethodException and falling back to chmod.


[JG06] Keeping binary archives (such as NBMs) in test sources is poor style. Better to create the JAR from source
entries when the test is run. org.openide.util.test.TestFileUtils make this easy.


[JG07, to supersede JG03 and perhaps JG02] Do you really need MakeExecutableFilesList? I doubt it. Just

<chmod perm="a+x">
  <fileset dir="${cluster}" includes="${nbm.executable.files}"/>
</chmod>
...
<nbm ...>
  <executables dir="${cluster}" includes="${nbm.executable.files}"/>
</nbm>

should work.


[JG08] The chmod-executables target should be a dependency of the 'netbeans' target, not antcall'd from release. (Should
follow 'release' and 'netbeans-extra' in the dep list.)

Re JG01: I still think it is required if apisupport.harness (NetBeans Plugin Development) is installed from UC.
In that case the launchers/app.sh is required to be chmod`ed after installation, right? Or is it chmoded when the 
platform app is been build?

Re [JG02] Both comma and space separation is allowed. It is processed like ant`s "includes" attribute ("comma- or space-
separated list of patterns of files that must be included"). I`ll fix the documentation.

Re [JG03] We`ll do, thanks.


[JG04] I think BufferedReader would be a lot simpler and clearer in readExecutableFilesList. Not to mention that your
impl ignores encoding. Use UTF-8. Don't use String.trim() without a compelling reason.


Re [JG05] Sure, will check for File.setExecutable and fallback to chmod.

Re [JG06] Agree. We`ll do.


Re [JG07] Yes, I do think that it is required. In case nbm.executable.files is a pattern (like bin/**), then doing the 
chmod you suggested will set executable permissions on *all* files already available in ${cluster}/bin/ directory, not 
only the files that belong to the particular module. In other words, doing so can result in unnecessary executable 
permission setting. Even though it is does not sound critical now, but I think better to avoid that than to fix 
possible bugs.

Re [JG08] Thanks, will fix.

JG01 - no, it is not a+x even in a regular build. As I said, it is used only when a platform app is built, so the
harness script takes care of it.


JG07 - just set the property to those files your module actually owns. I don't think setting it to "bin/" is necessary
in any situation.

Attaching the patch and bundle with all fixes due to Jesse`s comments except JG06 (postponed, we have a few nbms 
already in test sources - not critical for this issue, will work on it some time later hopefully).

Created attachment 85994 [details]
patch including fixes due to Jesse`s review

Created attachment 85995 [details]
hg bundle with fixes due to Jesse`s review

core-main #f2ec4ce487be

As an addendum to JG07, all this:

<target name="-init-executables"
depends="-check-executables,-define-executables-reference-defined,-define-executables-reference-undefined"/>
<target name="-check-executables">
    <condition property="has.executables">
        <and>
            <isset property="nbm.executable.files"/>
            <not>
               <equals arg1="${nbm.executable.files}" arg2=""/>
            </not>
        </and>
    </condition>
</target>
<target name="-define-executables-reference-defined" if="has.executables">
    <fileset dir="${cluster}" id="module.executable.files" includes="${nbm.executable.files}"/>
</target>
<target name="-define-executables-reference-undefined" unless="has.executables">
    <fileset dir="${cluster}" id="module.executable.files" excludes="**"/>
</target>

can probably be replaced with just

<target name="-init-executables">
    <property name="module.executable.files" value="[NOTHING]"/>
    <fileset dir="${cluster}" id="module.executable.files" includes="${nbm.executable.files}"/>
</target>

Integrated into 'main-golden', will be available in build *200908120201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/f2ec4ce487be
User: Dmitry Lipin <dlipin@netbeans.org>
Log: Issue #24357 ModuleUpdater should set file permissions on Unix

#309e1ad6a73d, 1d736701d1bd

Integrated into 'main-golden', will be available in build *200908210201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/1d736701d1bd
User: Dmitry Lipin <dlipin@netbeans.org>
Log: Fix property name (Issue #24357)

I have tried to remove chmod from cnd/build.xml by replacing with nbm.executable.files
But it doesn't work, because chmod-executables in harness doesn't work (tried with -v):
chmod-executables:
Skipped because property 'has.executables' not set.

dlipin probably forgot:

diff --git a/nbbuild/templates/common.xml b/nbbuild/templates/common.xml
--- a/nbbuild/templates/common.xml
+++ b/nbbuild/templates/common.xml
@@ -254,8 +254,7 @@
         </copy>        
     </target>
 
-    
-    <target name="chmod-executables" depends="-init-executables" if="has.executables">
+    <target name="chmod-executables" depends="-init-executables">
         <chmod perm="a+x" dir="${cluster}">
             <fileset refid="module.executable.files"/>
         </chmod>

still doesn't work.
Has anybody tried it?

Info/executable.list is generated correctly, but during build chmod is not executed.

Btw, perm="a+x" could be not enough.
perm="a+rx" is better (to prevent umask based problems of access denied do to lack of read access)

All files installed by AU should be readable to begin with, which is why only +x is necessary.

agree, but target chmod-executables is used not by installer. it is used during netbeans build => umask of user can
affect the run of shared built netbeans by other users

Indeed, I forgot it. Fixed in core-main#e52b18db8a05. Thanks, Jesse!

Vladimir, if that fix still does not work for you, please attach the ant verbose (-v) log and the value of your 
nbm.executable.files property.

I have
nbm.executable.files=bin/dorun.sh

It doesn't work.
....
bin/dorun.sh added as bin/dorun.sh doesn't exist.
bin/stdouterr.bat added as bin/stdouterr.bat doesn't exist.
bin/stdouterr.sh added as bin/stdouterr.sh doesn't exist.
No sources found.
Copying 3 files to /home/vv159170/devarea/trunk/nbbuild/netbeans/cnd2
Copying /home/vv159170/devarea/trunk/cnd/release/bin/stdouterr.sh to
/home/vv159170/devarea/trunk/nbbuild/netbeans/cnd2/bin/stdouterr.sh
Copying /home/vv159170/devarea/trunk/cnd/release/bin/dorun.sh to
/home/vv159170/devarea/trunk/nbbuild/netbeans/cnd2/bin/dorun.sh
Copying /home/vv159170/devarea/trunk/cnd/release/bin/stdouterr.bat to
/home/vv159170/devarea/trunk/nbbuild/netbeans/cnd2/bin/stdouterr.bat
release:
-init-executables:
Override ignored for property "nbm.executable.files"
chmod-executables:
Current OS is Linux
verify-class-linkage:
....

Integrated into 'main-golden', will be available in build *200908260201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/e52b18db8a05
User: Dmitry Lipin <dlipin@netbeans.org>
Log: Issue #24357 ModuleUpdater should set file permissions on Unix

Should be fixed now in 
http://hg.netbeans.org/core-main/rev/c031e5372024
Sorry for inconvenience.

Integrated into 'main-golden', will be available in build *200908261401* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/c031e5372024
User: Dmitry Lipin <dlipin@netbeans.org>
Log: Issue #24357 ModuleUpdater should set file permissions on Unix

Integrated into 'main-golden', will be available in build *201204250400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/7251aab540a1
User: Jesse Glick <jglick@netbeans.org>
Log: NetBeansInOSGi near-equivalent of #24357 (exec bits on files unpacked from NBM fileset).