tycho-p2-director:director: Fix handling of destination on macOS

[sratz]

No description provided.

[laeubi]

Can we probably have a new testcase instead of adjusting the existing one? Both "variants" should work in my opinion.

[github-actions]

Test Results

  591 files    591 suites   4h 31m 50s ⏱️
  413 tests   406 ✅  7 💤 0 ❌
1 239 runs  1 217 ✅ 22 💤 0 ❌

Results for commit ffd4114.

♻️ This comment has been updated with latest results.

[laeubi]

Thanks for working on this, this looks good in general just some small remarks.

[laeubi]

@sratz I plan to do a bugfix release maybe end of month, do you think we can include this one here?

[sratz]

@sratz I plan to do a bugfix release maybe end of month, do you think we can include this one here?

Yes, I think that should be possible, I am currently looking for a solution to this issue again.

[sratz]

I thought about this some more and changed the pull request again to what you initially proposed in the issue.

The processing of work in tycho-surefire-plugin and destination in tycho-p2-director-plugin is now consistent and the same rules apply.

I think the benefits of applying this in some standard scenario (e.g. https://github.com/eclipse-tycho/tycho/blob/d23230d9ea1c03e64184f3c69bfe69499e815ef8/tycho-its/projects/surefire.p2InstalledRuntime/extProductTestDirector/pom.xml) outweigh the fact that tycho-p2-director-plugin:director is not a drop-in-replacement for the legacy eclipse -application org.eclipse.equinox.p2.director -destination <destination> ....

I tried to document these peculiarities of path name handling in the mojo parameter descriptions.

[laeubi]

Looks fine and tests are green, @sratz if you are happy with that let me know and I'll put this on the merge train!

[sratz]

Looks fine and tests are green, @sratz if you are happy with that let me know and I'll put this on the merge train!

I have one more question:

I am trying to understand the intention behind the installAllEnvironments option:

tycho/tycho-surefire/tycho-surefire-plugin/src/main/java/org/eclipse/tycho/surefire/AbstractEclipseTestMojo.java

Line 700 in 95b912e

if (installAllEnvironments) {

Was the idea to run the director N times and install IUs for all platforms into the same product/installation?

Has this ever worked?

The way I see it, the environment options (os/ws/arch) end up in the director call inside the IProfile, but only if that profile does not exist, yet:

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

Lines 824 to 844 in c66c628

	private IProfile initializeProfile() throws CoreException {
	IProfile profile = getProfile();
	if (profile == null) {
	if (destination == null)
	missingArgument("destination"); //$NON-NLS-1$
	if (flavor == null)
	flavor = System.getProperty("eclipse.p2.configurationFlavor", FLAVOR_DEFAULT); //$NON-NLS-1$
	Map<String, String> props = new HashMap<>();
	props.put(IProfile.PROP_INSTALL_FOLDER, destination.toString());
	if (bundlePool == null)
	props.put(IProfile.PROP_CACHE,
	sharedLocation == null ? destination.getAbsolutePath() : sharedLocation.getAbsolutePath());
	else
	props.put(IProfile.PROP_CACHE, bundlePool.getAbsolutePath());
	if (roamingProfile)
	props.put(IProfile.PROP_ROAMING, Boolean.TRUE.toString());
	String env = getEnvironmentProperty();
	if (env != null)
	props.put(IProfile.PROP_ENVIRONMENTS, env);

So AFAIK installAllEnvironments is a no-op, and just runs the (same) director multiple times.

I am asking here, because if we could remove that option, it would simplify the handling of the target env even more inside ProvisionedInstallationBuilder

From what I can tell installAllEnvironments is not used in production anywhere.

[laeubi]

Was the idea to run the director N times and install IUs for all platforms into the same product/installation?
Has this ever worked?

Yes but I think it was actually in the end not that useful.

So AFAIK installAllEnvironments is a no-op, and just runs the (same) director multiple times.

the difference is if you have native fragments, you can install all of them while this si not possible with one classic call.

I am asking here, because if we could remove that option, it would simplify the handling of the target env even more inside ProvisionedInstallationBuilder

Yes I think we can remove that option.

[sratz]

the difference is if you have native fragments, you can install all of them while this si not possible with one classic call.

When I follow the information flow of os/ws/arch, I don't see how that would've ever worked in practice:

• org.eclipse.tycho.surefire.provisioning.ProvisionedInstallationBuilder.executeDirector(TargetEnvironment)
• org.eclipse.tycho.p2.tools.director.shared.DirectorRuntime.Command.setEnvironment(TargetEnvironment)
• Director arguments -p2.os / -p2.ws / -p2.arch

And then inside

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

Lines 824 to 844 in c66c628

	private IProfile initializeProfile() throws CoreException {
	IProfile profile = getProfile();
	if (profile == null) {
	if (destination == null)
	missingArgument("destination"); //$NON-NLS-1$
	if (flavor == null)
	flavor = System.getProperty("eclipse.p2.configurationFlavor", FLAVOR_DEFAULT); //$NON-NLS-1$
	Map<String, String> props = new HashMap<>();
	props.put(IProfile.PROP_INSTALL_FOLDER, destination.toString());
	if (bundlePool == null)
	props.put(IProfile.PROP_CACHE,
	sharedLocation == null ? destination.getAbsolutePath() : sharedLocation.getAbsolutePath());
	else
	props.put(IProfile.PROP_CACHE, bundlePool.getAbsolutePath());
	if (roamingProfile)
	props.put(IProfile.PROP_ROAMING, Boolean.TRUE.toString());
	String env = getEnvironmentProperty();
	if (env != null)
	props.put(IProfile.PROP_ENVIRONMENTS, env);

the only place where this information is used again is when creating a new profile, when it does not does not yet exist.

So, the first org.eclipse.tycho.surefire.provisioning.ProvisionedInstallationBuilder.install(TargetEnvironment) makes use of the environment, when creating the IProfile, but all the subsequent calls don't and they essentially just repeat the first invocation.

Yes I think we can remove that option.

Ok, then I'll do that in a separate pull-request to clean things up.

I think we can then merge this one.

[laeubi]

So, the first org.eclipse.tycho.surefire.provisioning.ProvisionedInstallationBuilder.install(TargetEnvironment) makes use of the environment, when creating the IProfile, but all the subsequent calls don't and they essentially just repeat the first invocation.

Yes the "key" was a new profile each time so they end up installed in the folder but as said in the end this was not really something useful at all...

Ok, then I'll do that in a separate pull-request to clean things up.

Whatever you think fits best if it make things easier you can also do it in this PR.

[sratz]

Whatever you think fits best if it make things easier you can also do it in this PR.

Then I'll do it here. Also keeps the discussions together in one issue/PR.

[laeubi]

Many thanks!

[eclipse-tycho-bot]

💚 All backports created successfully

Status	Branch	Result
✅	tycho-4.0.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details