Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 325301

Summary: Various files missing in branded executable IUs
Product: z_Archived Reporter: Michal Ruzicka <michal.ruza>
Component: BuckminsterAssignee: buckminster.core-inbox <buckminster.core-inbox>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: thomas
Version: unspecified   
Target Milestone: ---   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
org.eclipse.buckminster.pde-branding_refactoring.patch
none
org.eclipse.buckminster.pde-branding_refactoring_v2.patch
none
org.eclipse.buckminster.pde-branding_refactoring_v3.patch none

Description Michal Ruzicka CLA 2010-09-14 19:38:58 EDT 
STEPS TO REPRODUCE
1) create a *.product definition file, make sure you specify an icon file for Mac and/or Linux in it
2) invoke Buckminster's "site.p2" action on the project containing the *.product file while making sure the platform properties are set to wildcards:
target.ws=*
target.os=*
target.arch=*

ACTUAL RESULT
The resulting P2 repository contains a binary IU for each supported combination of ws, os, arch; this IU is supposed to contain launcher files for the specific ws, os, arch combination. But many of these IUs are missing various files, notably:
- the Mac IUs are missing launcher icons, and branded launcher.ini file (they do contain these files, but they are apparently not created by branding of the template files)
- the Linux IUs are missing launcher icons

EXPECTED RESULT
All of the binary IUs contain all files appropriate to respective platform.

ADDITIONAL INFO
The branding works like this:
1) a feature containing templates of launchers for all supported ws, os, arch combinations (e.g. org.eclipse.equinox.executable) is located

2) a temporary copy of the template for the ws, os, arch combination which is currently being built is created; the contents of the temporary template copy is reflected in an instance of org.eclipse.equinox.internal.p2.publisher.eclipse.ExecutablesDescriptor class
see:
org.eclipse.equinox.p2.publisher.eclipse.EquinoxExecutableAction.brandExecutables()

3) the branding process is invoked on the temporary template copy; this branding process applies all the branding to the files in the temporary template copy, correctly creating all the files appropriate for the current platform (including the icons); but the branding process does not have the corresponding instance of the org.eclipse.equinox.internal.p2.publisher.eclipse.ExecutablesDescriptor class at hand to reflect the changes it makes there
see:
org.eclipse.buckminster.pde.tasks.EquinoxExecutableAction.fullBrandExecutables()
org.eclipse.buckminster.pde.tasks.BrandingIron

4) after the branding process finishes no real attempt is made to detect the changes it made and so most of them are *NOT* reflected (though some of the changes are expected and thus reflected) in the corresponding instance of the org.eclipse.equinox.internal.p2.publisher.eclipse.ExecutablesDescriptor class
see:
org.eclipse.buckminster.pde.tasks.EquinoxExecutableAction.fullBrandExecutables()

5) ultimately the instance of the org.eclipse.equinox.internal.p2.publisher.eclipse.ExecutablesDescriptor class is used to create the resulting binary IU

From what's summarized above it follows that despite the branding process does its job right the changes it makes are not reflected in the resulting IU as the corresponding instance of org.eclipse.equinox.internal.p2.publisher.eclipse.ExecutablesDescriptor class is not updated accordingly.
Comment 1 Michal Ruzicka CLA 2010-09-14 19:49:15 EDT 
Created attachment 178891 [details]
org.eclipse.buckminster.pde-branding_refactoring.patch

The attached patch gives the branding code in org.eclipse.buckminster.pde.tasks.BrandingIron access to the instance of org.eclipse.equinox.internal.p2.publisher.eclipse.ExecutablesDescriptor describing the launcher template to be branded.
The patch further makes sure that any changes made to the template are immediately reflected in the corresponding org.eclipse.equinox.internal.p2.publisher.eclipse.ExecutablesDescriptor descriptor.
Comment 2 Thomas Hallgren CLA 2010-09-15 01:27:19 EDT 
ASAICS, this patch will fail if executed on a windows machine. In particular, there's an if that conditionally links stuff if the os is not Windows but there's no else with a fallback solution in case it is.

There's some calls to *nix specific chmod on executables. Isn't that something that p2 is supposed to do during install using touchpoints?
Comment 3 Michal Ruzicka CLA 2010-09-15 19:30:46 EDT 
(In reply to comment #2)
> ASAICS, this patch will fail if executed on a windows machine. In particular,
> there's an if that conditionally links stuff if the os is not Windows but
> there's no else with a fallback solution in case it is.
> 
There is no "else" clause, but still the code falls back to do a copy of the resource if the link command fails; further the copy is done on Windows* systems right away (without first trying the link command). To be able to perform the copy operation in both of these situations with a single code block I could not wrap it in the "else" clause of the "if" statement checking the result of the link command. Instead I've created a named block which is simply exited if the link command succeeds. But in case the link command fails (or if it is not even attempted when on Windows) the execution of the named block continues and reaches the code performing the copy.

> There's some calls to *nix specific chmod on executables. Isn't that something
> that p2 is supposed to do during install using touchpoints?
>
You are right, calling chmod is most probably useless in the branding code. Given the fact that whatever the branding code creates is eventually packed into a jar file, anything the chmod call might have set is lost.
Attaching a new version of the patch without the chmod calls.
Comment 4 Michal Ruzicka CLA 2010-09-15 19:35:29 EDT 
Created attachment 178990 [details]
org.eclipse.buckminster.pde-branding_refactoring_v2.patch

Take 2 of the barnding refactoring patch:
- removed useless executions of chmod
- applies cleanly to the helios-maintenance branch
Comment 5 Thomas Hallgren CLA 2010-09-16 02:50:59 EDT 
Looks good. If this works for you and produces Mac products that are OK, both when you run on Linux, Mac, and Windows ;-), then please commit this patch to the helios-maintenance branch.

Once that's in, can you please start a new Hudson build?
Comment 6 Thomas Hallgren CLA 2010-09-23 01:38:38 EDT 
Michal, did you commit your changes?
Comment 7 Michal Ruzicka CLA 2010-09-30 08:08:18 EDT 
Created attachment 179944 [details]
org.eclipse.buckminster.pde-branding_refactoring_v3.patch

Improved version of the patch which (in addition to changes introduced by the previous version of the patch) removes the "root" attribute from the org.eclipse.buckminster.pde.tasks.BrandingIron class. The attribute is not necessary as the org.eclipse.equinox.internal.p2.publisher.eclipse.ExecutablesDescriptor which is passed down to all the methods as a parameter contains the same information.
Comment 8 Michal Ruzicka CLA 2010-09-30 08:25:16 EDT 
I tested the changes captured in org.eclipse.buckminster.pde-branding_refactoring_v3.patch on:
- Windows XP
- Mac OS X 10.5
- openSUSE Linux 11.3 (x86_64)
Both with and without the org.eclipse.equinox.executable feature present in the target platform.
Everything worked as expected so I've committed the patch in revision 11582 to the helios-maintenance branch.
Comment 9 Michal Ruzicka CLA 2010-09-30 08:32:03 EDT 
Is there a Hudson job for building Buckminster on the new hardware (hudson.eclipse.org) yet?
Comment 10 Thomas Hallgren CLA 2010-09-30 08:37:33 EDT 
No, I didn't bother to move it. Wanted the storm to blow over first. I think most problems are resolved by now so now is probably a good time.
Comment 11 Michal Ruzicka CLA 2010-12-15 17:01:38 EST 
(In reply to comment #8)
The issue is actually fixed since the time of the above comment, so just closing it.