Checking for value instead of key in

[vogella]

RemoteArtifactRepositoryManagerAgentFactory

Fixes #3375

[vogella]

cc @Phillipus please review

[Phillipus]

cc @Phillipus please review

Please see comment here

[laeubi]

@Phillipus can you make sure you specify the profile on the commandline with -P or even try without a profile?

[Phillipus]

@Phillipus can you make sure you specify the profile on the commandline with -P or even try without a profile?

It should be reading the active profile from settings.xml right? So no need to specify it with -P.

Is mavenContext.getSession().getSystemProperties().getProperty(key) and mavenContext.getSession().getUserProperties().getProperty(key) the correct way to read from settings.xml?

[github-actions]

Test Results

  579 files  ±0    579 suites  ±0   3h 32m 57s ⏱️ - 7m 0s
  383 tests ±0    375 ✅  - 1   7 💤 ±0  1 ❌ +1 
1 149 runs  ±0  1 126 ✅  - 1  22 💤 ±0  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 97a1805. ± Comparison against base commit d099c82.

[laeubi]

Profiles sometimes working in a "surprising" way in maven... so its up to you to just try it out if it is such a case here and I always would test with the most simple setup first.

[Phillipus]

Profiles sometimes working in a "surprising" way in maven... so its up to you to just try it out if it is such a case here and I always would test with the most simple setup first.

The point I'm trying to make is that the PR is trying to do the following (as documented here):

1. Read the property from System.getProperty(key)
   or
2. Read the property from the user's .m2/settings.xml file

(1) works, but (2) doesn't.

For (2), this has to be declared in a profile block because there is no other way to do that in settings.xml.

So the simple question is, is @vogella 's code wrong?

[Phillipus]

Looking at 7e6d595 it used to be MavenContext#getSessionProperties()

@vogella Can you help here? I'm just clutching at straws here trying to help out and I don't know this code.

[vogella]

Unfortunately I'm currently don't have access to an Eclipse setup, the last changes were done with VsCode due to their urgency.

[Phillipus]

Unfortunately I'm currently don't have access to an Eclipse setup, the last changes were done with VsCode due to their urgency.

I'm in much the same position here, using VS Code to do trial and error changes on the Tycho source, then compiling and installing it into my .m2 repo to test.

I can't do any more testing but what I've found is:

LegacySupport.getSession().getSystemProperties().getProperty(key) doesn't read a property from the settings.xml file.

LegacySupport..getSession().getUserProperties().getProperty(key) doesn't read a property from the settings.xml file.

What does read a property from the settings.xml file is MavenContext.getSessionProperties().getProperty(key) but, for some reason, that can't be used in RemoteArtifactRepositoryManagerAgentFactory because it can't be found on the classpath.

[laeubi]

MavenContext.getSessionProperties().getProperty(key)

Is a wrapper to MavenContextConfigurator.getGlobalProperties(MavenSession)

tycho/tycho-core/src/main/java/org/eclipse/tycho/osgi/configuration/MavenContextConfigurator.java

Lines 49 to 65 in d099c82

	static Properties getGlobalProperties(MavenSession session) {
	Properties globalProps = new Properties();
	// 1. system
	globalProps.putAll(session.getSystemProperties());
	Settings settings = session.getSettings();
	// 2. active profiles
	Map<String, Profile> profileMap = settings.getProfilesAsMap();
	for (String profileId : settings.getActiveProfiles()) {
	Profile profile = profileMap.get(profileId);
	if (profile != null) {
	globalProps.putAll(profile.getProperties());
	}
	}
	// 3. user
	globalProps.putAll(session.getUserProperties());
	return globalProps;
	}

so whats missing here is that RemoteArtifactRepositoryManagerAgentFactory#getMirrorProperty needs to iterate the properties of the active profiles like in that method.

Unfortunately I'm currently don't have access to an Eclipse setup, the last changes were done with VsCode due to their urgency.

...

I'm in much the same position here, using VS Code to do trial and error changes on the Tycho source, then compiling and installing it into my .m2 repo to test.

Due to the nature of Tycho that's also the way to go with Eclipse as well so there is not really a difference, compile/install and then start with mvnDebug <...options..> or even use "System.out debugging", you can't really start it from within your IDE.

Beside that, if this feature is important to anyone please consider provide an integration-test to demonstrate the feature as it seems this part is obviously not covered at the moment and therefore considered undefined behavior.

[Phillipus]

Beside that, if this feature is important to anyone please consider provide an integration-test to demonstrate the feature as it seems this part is obviously not covered at the moment and therefore considered undefined behavior.

@vogella I'm afraid I can't work on this one. Will you able to take this further? Perhaps enough info in the above comment to make it work with the settings.xml file. If not, might be a good idea to revert the original PR (and in the 4.0.x branch too)?

[Phillipus]

so whats missing here is that RemoteArtifactRepositoryManagerAgentFactory#getMirrorProperty needs to iterate the properties of the active profiles like in that method.

Indeed. Doing this and it works.

@vogella I created the following working code. Would you like to take this, improve it if needed, and apply it to your PR:

/*******************************************************************************
 * Copyright (c) 2022 Christoph Läubrich and others.
 * This program and the accompanying materials
 * are made available under the terms of the Eclipse Public License 2.0
 * which accompanies this distribution, and is available at
 * https://www.eclipse.org/legal/epl-2.0/
 *
 * SPDX-License-Identifier: EPL-2.0
 *
 * Contributors:
 *    Christoph Läubrich - initial API and implementation
 *******************************************************************************/
package org.eclipse.tycho.p2maven.transport;

import java.util.Map;
import java.util.Properties;

import org.apache.maven.execution.MavenSession;
import org.apache.maven.plugin.LegacySupport;
import org.apache.maven.settings.Profile;
import org.apache.maven.settings.Settings;
import org.codehaus.plexus.component.annotations.Component;
import org.codehaus.plexus.component.annotations.Requirement;
import org.codehaus.plexus.logging.Logger;
import org.eclipse.equinox.internal.p2.artifact.repository.ArtifactRepositoryComponent;
import org.eclipse.equinox.p2.core.IProvisioningAgent;
import org.eclipse.equinox.p2.core.spi.IAgentServiceFactory;
import org.eclipse.equinox.p2.repository.artifact.IArtifactRepositoryManager;
import org.eclipse.tycho.IRepositoryIdManager;
import org.eclipse.tycho.version.TychoVersion;

@Component(role = IAgentServiceFactory.class, hint = "org.eclipse.equinox.p2.repository.artifact.IArtifactRepositoryManager")
public class RemoteArtifactRepositoryManagerAgentFactory implements IAgentServiceFactory {

	@Requirement
	Logger logger;

	@Requirement
	IRepositoryIdManager repositoryIdManager;

	@Requirement
	MavenAuthenticator authenticator;

	@Requirement
	MavenSession mavenSession;

	@Override
	public Object createService(IProvisioningAgent agent) {
		IArtifactRepositoryManager plainRepoManager = (IArtifactRepositoryManager) new ArtifactRepositoryComponent()
				.createService(agent);
		if (getDisableP2MirrorsConfiguration()) {
			plainRepoManager = new P2MirrorDisablingArtifactRepositoryManager(plainRepoManager, logger);
		}
		return new RemoteArtifactRepositoryManager(plainRepoManager, repositoryIdManager, authenticator);
	}

	private boolean getDisableP2MirrorsConfiguration() {
		String deprecatedKey = "tycho.disableP2Mirrors";
		String deprecatedValue = getGlobalProperties().getProperty(deprecatedKey);

		if (deprecatedValue != null) {
			logger.info("Using " + deprecatedKey
					+ " to disable P2 mirrors is deprecated, use the property eclipse.p2.mirrors instead, see https://tycho.eclipseprojects.io/doc/"
					+ TychoVersion.getTychoVersion() + "/SystemProperties.html for details.");
			return Boolean.parseBoolean(deprecatedValue);
		}

		String key = "eclipse.p2.mirrors";
		String value = getGlobalProperties().getProperty(key);

		if (value != null) {
			// eclipse.p2.mirrors false -> disable mirrors
			return !Boolean.parseBoolean(value);
		}
		return false;
	}

	private Properties getGlobalProperties() {
		Properties globalProps = new Properties();
		// 1. system
		globalProps.putAll(mavenSession.getSystemProperties());
		Settings settings = mavenSession.getSettings();
		// 2. active profiles
		Map<String, Profile> profileMap = settings.getProfilesAsMap();
		for (String profileId : settings.getActiveProfiles()) {
			Profile profile = profileMap.get(profileId);
			if (profile != null) {
				globalProps.putAll(profile.getProperties());
			}
		}
		// 3. user
		globalProps.putAll(mavenSession.getUserProperties());
		return globalProps;
	}
}

[laeubi]

@Phillipus your code creates the properties twice ... also instead of creating a new properties opject, its is simply enough to check the inidivial ones for the property of interests (you then need to check in reverse order)

[Phillipus]

@Phillipus your code creates the properties twice ... also instead of creating a new properties opject, its is simply enough to check the inidivial ones for the property of interests (you then need to check in reverse order)

My aim here is to help @vogella with some POC code. I didn't really want to be the guy who supplies the PR with full tests!

[vogella]

@Phillipus your code creates the properties twice ... also instead of creating a new properties opject, its is simply enough to check the inidivial ones for the property of interests (you then need to check in reverse order)

My aim here is to help @vogella with some POC code. I didn't really want to be the guy who supplies the PR with full tests!

Working currently outside the Eclipse world, so if you could provide a fix or revert that would be great.

[Phillipus]

Working currently outside the Eclipse world, so if you could provide a fix or revert that would be great.

I seem to have been sucked into a PR that I'm not best qualified to fulfil. My contributions above were intended just to help out with the PR with POC code. As far as I can see, the present state of RemoteArtifactRepositoryManagerAgentFactory is unstable, and perhaps @laeubi might revert?

[laeubi]

@Phillipus @vogella I created now:

• Also consider profile properties when looking for disable mirror keys #3384

please continue there.