tycho-p2-director:director uses wrong directory when run on macOS

[sratz]

When tycho-p2-director:director is executed on macOS, the destination contains an additional Contents/Eclipse/ directory.

This seems to have been introduced with dc99997:

tycho/tycho-p2-director-plugin/src/main/java/org/eclipse/tycho/plugins/p2/director/DirectorMojo.java

Line 364 in 62f83cf

DirectorRuntime.getDestination(destination, TargetEnvironment.getRunningEnvironment()));

But this is incorrect. The running environment is not relevant for the target destination.

I'll provide an extension to the existing integration test that should fail when the test itself is executed on macOS.

[laeubi]

But this is incorrect. The running environment is not relevant for the target destination.

It depends a bit how should director know the "target"? I would think about we use the configurable

tycho/tycho-p2-director-plugin/src/main/java/org/eclipse/tycho/plugins/p2/director/DirectorMojo.java

Line 263 in 62f83cf

private String p2os;

if given or check if the profile gives some hints but beside this its hard to guess.

[sratz]

If -destination is given a parameter that ends in .app, and p2os is macosx, the director already handles this itself.

[sratz]

tycho/tycho-core/src/main/java/org/eclipse/tycho/p2tools/copiedfromp2/DirectorApplication.java

Lines 925 to 932 in 62f83cf

	private void adjustDestination() {
	// Detect the desire to have a bundled mac application and tweak the environment
	if (destination == null)
	return;
	if (org.eclipse.osgi.service.environment.Constants.OS_MACOSX.equals(os)
	&& destination.getName().endsWith(".app")) //$NON-NLS-1$
	destination = new File(destination, "Contents/Eclipse"); //$NON-NLS-1$
	}

So in DirectorMojo, the destination should be passed in to -destination without any pre-processing.

[laeubi]

If -destination is given a parameter that ends in .app, and p2os is macosx, the director already handles this itself.

But this does not work if destination if a generic folder and one wants to use it on multiple platforms.

[sratz]

But this does not work if destination if a generic folder and one wants to use it on multiple platforms.

But that is exactly the way the stand-alone director (called via eclipse-run / ant) used to work.

Why would we want to change this behavior when called directly from tycho?

[laeubi]

Hard to tell but Tycho automatically detects this already for e.g. the p2 installed runtime in test so it seems at laest valid here as well.

For example org.eclipse.tycho.p2.tools.director.shared.DirectorRuntime.getDestination(File, TargetEnvironment) already checks if it is a valid layout and if that the case passes the path unchanged, so maybe one should just take into account the p2os and then everything will work as expsected.

[sratz]

Hard to tell but Tycho automatically detects this already for e.g. the p2 installed runtime in test so it seems at laest valid here as well.
For example org.eclipse.tycho.p2.tools.director.shared.DirectorRuntime.getDestination(File, TargetEnvironment) already checks if it is a valid layout and if that the case passes the path unchanged, so maybe one should just take into account the p2os and then everything will work as expsected.

But that would mean that DirectorMojo is not a 1:1 drop-in replacement for a

eclipse -application org.eclipse.equinox.p2.director -destination <destination> ...

stand-alone invocation, because <destination> is modified by Tycho, and the original logic of dealing with macOS app bundles defined in

tycho/tycho-core/src/main/java/org/eclipse/tycho/p2tools/copiedfromp2/DirectorApplication.java

Lines 925 to 932 in 62f83cf

	private void adjustDestination() {
	// Detect the desire to have a bundled mac application and tweak the environment
	if (destination == null)
	return;
	if (org.eclipse.osgi.service.environment.Constants.OS_MACOSX.equals(os)
	&& destination.getName().endsWith(".app")) //$NON-NLS-1$
	destination = new File(destination, "Contents/Eclipse"); //$NON-NLS-1$
	}

would never be hit anymore.

I think this is a bit counter-intuitive.

[sratz]

Also:

tycho/tycho-core/src/main/java/org/eclipse/tycho/p2/tools/director/shared/DirectorRuntime.java

Lines 81 to 86 in 62f83cf

	public static File getDestination(File baseLocation, TargetEnvironment env) {
	if (PlatformPropertiesUtils.OS_MACOSX.equals(env.getOs()) && !hasRequiredMacLayout(baseLocation)) {
	return new File(baseLocation, "Eclipse.app/Contents/Eclipse/");
	}
	return baseLocation;
	}

currently hard-codes Eclipse.app as the bundle name.

This does not work for custom products, eg. we are using

          <execution>
            <id>run-director-mac</id>
            <goals>
              <goal>director</goal>
            </goals>
            <phase>package</phase>
            <configuration>
              <destination>target/ABAPDevelopmenTools.app</destination>
              <p2os>macosx</p2os>
              <p2ws>cocoa</p2ws>
              <p2arch>x86_64</p2arch>
              
            </configuration>
          </execution>

[laeubi]

stand-alone invocation, because is modified by Tycho, and the original logic of dealing with macOS app bundles defined in

I'm not sure I really understand if it makes a difference where in Tycho code this check is performed, so maybe we should just remove that check altogether in DirectorApplication?

This does not work for custom products, eg. we are using

This should be covered by hasRequiredMacLayout, but obviously there is no test for that currently.

[laeubi]

@sratz just a general comment, the main "issue" maybe is that there is only little documentation on the DirectorApp usage regarding mac and I just added it to be more convenient, but it seems you have some experience, so what do you think about we make

<p2os>...</p2os>
<p2ws>...</p2ws>
<p2arch>..</p2arch>

somehow required and if not given forcefully set it to the running environment?

[sratz]

stand-alone invocation, because is modified by Tycho, and the original logic of dealing with macOS app bundles defined in

I'm not sure I really understand if it makes a difference where in Tycho code this check is performed, so maybe we should just remove that check altogether in DirectorApplication?

Well there are two options:

1. We change the semantics and make
   DirectorMojo
   explicitly different in behavior to
   eclipse -application org.eclipse.equinox.p2.director -destination <destination> ....
   That means instead of
   <destination>target/ABAPDevelopmenTools.app</destination>
   we would have to use
   <destination>target/ABAPDevelopmenTools.app/Contents/Eclipse</destination>
   in the pom.xml so hasRequiredMacLayout understands our intention and to get the same directory layout result.
   (And hasRequiredMacLayout would need to be fixed to handle arbitrary app bundle names).
2. We keep the old semantics and revert line

   tycho/tycho-p2-director-plugin/src/main/java/org/eclipse/tycho/plugins/p2/director/DirectorMojo.java

   Line 364 in 62f83cf

   DirectorRuntime.getDestination(destination, TargetEnvironment.getRunningEnvironment()));

   again.

@sratz just a general comment, the main "issue" maybe is that there is only little documentation on the DirectorApp usage regarding mac and I just added it to be more convenient, ...

Yes, that's a great addition to Tycho and the original documentation is indeed lacking. So getting the original
eclipse -application org.eclipse.equinox.p2.director -destination <destination> ...
to work for all scenarios / platforms is already some obstacle :)
That's also why my vote would be to go with option 2) to allow for easy migration / compatibility / less surprises.

... but it seems you have some experience, so what do you think about we make

<p2os>...</p2os>
<p2ws>...</p2ws>
<p2arch>..</p2arch>

somehow required and if not given forcefully set it to the running environment?

This is relevant in case of option 1) above, in which case, yes, <p2os> & co. must be considered if given. If not given, a fallback could be made to the running environment.

[laeubi]

I think that sounds reasonable, do you like to add a testcase for the <destination>target/ABAPDevelopmenTools.app</destination> case as well? Sadly I have no access to any mac so its for me more flying blindly and hoping the PR validation shows any mac issues and if so debugging becomes a nightmare.

So to summarize I think as you suggested:

1. remove preprocessing of destination in DirectorMojo
2. if any p2xx is given all of them must be specified or mojo throws an error
3. if no p2xx is given, we derive them from running environment
4. Add more test-cases for different usages and maybe some more javadoc to the Mojo and its parameters

@sratz do you like to give it a try?

[sratz]

So to summarize I think as you suggested:

1. remove preprocessing of destination in DirectorMojo
2. if any p2xx is given all of them must be specified or mojo throws an error
3. if no p2xx is given, we derive them from running environment
4. Add more test-cases for different usages and maybe some more javadoc to the Mojo and its parameters

Yes, that sounds good.

@sratz do you like to give it a try?

Yes, I'll provide a pull-request (or rather update #3549).