Skip to content

[MNG-7556] Clean up notion between user properties and system properties #810

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
asfgit merged 1 commit into from
Oct 29, 2022
Merged

[MNG-7556] Clean up notion between user properties and system properties #810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions apache-maven/src/assembly/maven/conf/settings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ under the License.
-->
</pluginGroups>

<!-- TODO Since when can proxies be selected as depicted? -->
<!-- proxies
| This is a list of proxies which can be used on this machine to connect to the network.
| Unless otherwise specified (by system property or command-line switch), the first proxy
Expand Down Expand Up @@ -176,7 +177,7 @@ under the License.
|
| As noted above, profiles can be activated in a variety of ways. One way - the activeProfiles
| section of this document (settings.xml) - will be discussed later. Another way essentially
| relies on the detection of a system property, either matching a particular value for the property,
| relies on the detection of a property, either matching a particular value for the property,
| or merely testing its existence. Profiles can also be activated by JDK version prefix, where a
| value of '1.4' might activate a profile when the build is executed on a JDK version of '1.4.2_07'.
| Finally, the list of active profiles can be specified directly from the command line.
Expand Down Expand Up @@ -218,9 +219,9 @@ under the License.
-->

<!--
| Here is another profile, activated by the system property 'target-env' with a value of 'dev',
| which provides a specific path to the Tomcat instance. To use this, your plugin configuration
| might hypothetically look like:
| Here is another profile, activated by the property 'target-env' with a value of 'dev', which
| provides a specific path to the Tomcat instance. To use this, your plugin configuration might
| hypothetically look like:
|
| ...
| <plugin>
Expand Down
4 changes: 2 additions & 2 deletions api/maven-api-model/src/main/mdo/maven.mdo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2672,7 +2672,7 @@
<field>
<name>property</name>
<version>4.0.0+</version>
<description>Specifies that this profile will be activated when this system property is
<description>Specifies that this profile will be activated when this property is
specified.</description>
Comment on lines 2675 to 2676
Copy link
Copy Markdown
Member

@slawekjaranowski slawekjaranowski Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use project property for profile activation - right?
It can be user or system property.
Maybe it should be clear here?

Copy link
Copy Markdown
Member Author

@michael-o michael-o Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be checked...

Copy link
Copy Markdown
Member Author

@michael-o michael-o Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked PropertyProfileActivator. It DOES use user and then system properties. No project/model properties, but the passed ProfileActivationContext contains a getProjectProperties() method so we could make this happen.

<association>
<type>ActivationProperty</type>
Expand Down Expand Up @@ -2774,7 +2774,7 @@
activated. On the other hand, {@code exists} will test for the existence of the file and if it is
there, the profile will be activated.<br>
Variable interpolation for these file specifications is limited to {@code ${basedir}},
System properties and request properties.]]></description>
system properties and user properties.]]></description>
<fields>
<field>
<name>missing</name>
Expand Down
2 changes: 1 addition & 1 deletion api/maven-api-settings/src/main/mdo/settings.mdo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -789,7 +789,7 @@
<version>1.0.0+</version>
<description>
<![CDATA[
Specifies that this profile will be activated when this System property is specified.
Specifies that this profile will be activated when this property is specified.
]]>
</description>
<association>
Expand Down
2 changes: 1 addition & 1 deletion maven-compat/src/main/mdo/profiles.mdo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ under the License.
<name>property</name>
<version>1.0.0</version>
<description><![CDATA[
Specifies that this profile will be activated when this System property is specified.
Specifies that this profile will be activated when this property is specified.
]]></description>
<association>
<type>ActivationProperty</type>
Expand Down
2 changes: 1 addition & 1 deletion maven-core/src/main/java/org/apache/maven/eventspy/EventSpy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

/**
* A core extension to monitor Maven's execution. Typically, such an extension gets loaded into Maven by specifying the
* system property {@code maven.ext.class.path} on the command line. As soon as dependency injection is set up, Maven
* property {@code maven.ext.class.path} on the command line. As soon as dependency injection is set up, Maven
* looks up all implementers of this interface and calls their {@link #init(Context)} method. <em>Note:</em>
* Implementors are strongly advised to inherit from {@link AbstractEventSpy} instead of directly implementing this
* interface.
Expand Down
9 changes: 5 additions & 4 deletions ...src/main/java/org/apache/maven/internal/aether/DefaultRepositorySystemSessionFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ public DefaultRepositorySystemSession newRepositorySession( MavenExecutionReques
configProps.put( ConfigurationProperties.USER_AGENT, getUserAgent() );
configProps.put( ConfigurationProperties.INTERACTIVE, request.isInteractiveMode() );
configProps.put( "maven.startTime", request.getStartTime() );
// Resolver's ConfigUtils solely rely on config properties, that is why we need to add both here as well.
configProps.putAll( request.getSystemProperties() );
configProps.putAll( request.getUserProperties() );

Expand Down Expand Up @@ -288,14 +289,14 @@ else if ( !MAVEN_RESOLVER_TRANSPORT_AUTO.equals( transport ) )
+ MAVEN_RESOLVER_TRANSPORT_NATIVE + ", " + MAVEN_RESOLVER_TRANSPORT_AUTO );
}

session.setTransferListener( request.getTransferListener() );

session.setRepositoryListener( eventSpyDispatcher.chainListener( new LoggingRepositoryListener( logger ) ) );

session.setUserProperties( request.getUserProperties() );
session.setSystemProperties( request.getSystemProperties() );
session.setConfigProperties( configProps );

session.setTransferListener( request.getTransferListener() );

session.setRepositoryListener( eventSpyDispatcher.chainListener( new LoggingRepositoryListener( logger ) ) );

mavenRepositorySystem.injectMirror( request.getRemoteRepositories(), request.getMirrors() );
mavenRepositorySystem.injectProxy( session, request.getRemoteRepositories() );
mavenRepositorySystem.injectAuthentication( session, request.getRemoteRepositories() );
Expand Down
13 changes: 7 additions & 6 deletions maven-core/src/main/java/org/apache/maven/plugin/PluginParameterExpressionEvaluator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,9 @@
* <td>{@link MojoExecution#getMojoDescriptor()}.{@link MojoDescriptor#getPluginDescriptor()
* getPluginDescriptor()}</td></tr>
* <tr><td><code>plugin.*</code></td> <td></td> <td></td></tr>
* <tr><td><code>*</code></td> <td></td> <td>system properties</td></tr>
* <tr><td><code>*</code></td> <td></td> <td>user properties</td></tr>
* <tr><td><code>*</code></td> <td></td> <td>project properties</td></tr>
* <tr><td><code>*</code></td> <td></td> <td>system properties</td></tr>
* </table>
* <i>Notice:</i> <code>reports</code> was supported in Maven 2.x but was removed in Maven 3
*
Expand Down Expand Up @@ -368,7 +369,7 @@ else if ( expression.startsWith( "basedir" ) )
/*
* MNG-4312: We neither have reserved all of the above magic expressions nor is their set fixed/well-known (it
* gets occasionally extended by newer Maven versions). This imposes the risk for existing plugins to
* unintentionally use such a magic expression for an ordinary system property. So here we check whether we
* unintentionally use such a magic expression for an ordinary property. So here we check whether we
* ended up with a magic value that is not compatible with the type of the configured mojo parameter (a string
* could still be converted by the configurator so we leave those alone). If so, back off to evaluating the
* expression from properties only.
Expand All @@ -384,10 +385,10 @@ else if ( expression.startsWith( "basedir" ) )

if ( properties != null )
{
// We will attempt to get nab a system property as a way to specify a
// parameter to a plugins. My particular case here is allowing the surefire
// plugin to run a single test so I want to specify that class on the cli
// as a parameter.
// We will attempt to get nab a property as a way to specify a parameter
// to a plugin. My particular case here is allowing the surefire plugin
// to run a single test so I want to specify that class on the cli as
// a parameter.

value = properties.getProperty( expression );
}
Expand Down
11 changes: 6 additions & 5 deletions maven-core/src/main/java/org/apache/maven/plugin/PluginParameterExpressionEvaluatorV4.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
* <td>{@link MojoExecution#getMojoDescriptor()}.{@link MojoDescriptor#getPluginDescriptor()
* getPluginDescriptor()}</td></tr>
* <tr><td><code>plugin.*</code></td> <td></td> <td></td></tr>
* <tr><td><code>*</code></td> <td></td> <td>user properties</td></tr>
* <tr><td><code>*</code></td> <td></td> <td>system properties</td></tr>
* <tr><td><code>*</code></td> <td></td> <td>project properties</td></tr>
* </table>
Expand Down Expand Up @@ -376,7 +377,7 @@ else if ( expression.startsWith( "basedir" ) )
/*
* MNG-4312: We neither have reserved all of the above magic expressions nor is their set fixed/well-known (it
* gets occasionally extended by newer Maven versions). This imposes the risk for existing plugins to
* unintentionally use such a magic expression for an ordinary system property. So here we check whether we
* unintentionally use such a magic expression for an ordinary property. So here we check whether we
* ended up with a magic value that is not compatible with the type of the configured mojo parameter (a string
* could still be converted by the configurator so we leave those alone). If so, back off to evaluating the
* expression from properties only.
Expand All @@ -392,10 +393,10 @@ else if ( expression.startsWith( "basedir" ) )

if ( properties != null )
{
// We will attempt to get nab a system property as a way to specify a
// parameter to a plugins. My particular case here is allowing the surefire
// plugin to run a single test so I want to specify that class on the cli
// as a parameter.
// We will attempt to get nab a property as a way to specify a parameter
// to a plugin. My particular case here is allowing the surefire plugin
// to run a single test so I want to specify that class on the cli as
// a parameter.

value = properties.getProperty( expression );
}
Expand Down
13 changes: 11 additions & 2 deletions maven-core/src/main/java/org/apache/maven/project/artifact/MavenMetadataSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,8 +576,17 @@ private ProjectRelocation retrieveRelocatedProject( Artifact artifact, MetadataR
configuration.setValidationLevel( ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL );
configuration.setProcessPlugins( false );
configuration.setRepositoryMerging( ProjectBuildingRequest.RepositoryMerging.REQUEST_DOMINANT );
configuration.setSystemProperties( getSystemProperties() );
configuration.setUserProperties( new Properties() );
MavenSession session = legacySupport.getSession();
if ( session != null )
{
configuration.setSystemProperties( session.getSystemProperties() );
configuration.setUserProperties( session.getUserProperties() );
}
else
{
configuration.setSystemProperties( getSystemProperties() );
configuration.setUserProperties( new Properties() );
}
configuration.setRepositorySession( legacySupport.getRepositorySession() );

project = projectBuilder.build( pomArtifact, configuration ).getProject();
Expand Down
2 changes: 1 addition & 1 deletion maven-core/src/site/apt/getting-to-container-configured-mojos.apt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ Abstract

In order to really achieve this, we need mojo configurations (which are
provided both in terms of static expressions that are just looked up, and
in terms of user-provided configuration from system properties or the POM).
in terms of user-provided configuration from properties or the POM).
If these mojos are to be first-class components, the configuration from these
various sources must be consolidated and injected using the container.

Expand Down
10 changes: 8 additions & 2 deletions maven-embedder/src/main/java/org/apache/maven/cli/CLIManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,13 @@ public class CLIManager

public static final char BATCH_MODE = 'B';

public static final char SET_SYSTEM_PROPERTY = 'D';
public static final char SET_USER_PROPERTY = 'D';

/**
* @deprecated Use {@link #SET_USER_PROPERTY}
*/
@Deprecated
public static final char SET_SYSTEM_PROPERTY = SET_USER_PROPERTY;

public static final char OFFLINE = 'o';

Expand Down Expand Up @@ -123,7 +129,7 @@ public CLIManager()
options = new Options();
options.addOption( Option.builder( Character.toString( HELP ) ).longOpt( "help" ).desc( "Display help information" ).build() );
options.addOption( Option.builder( Character.toString( ALTERNATE_POM_FILE ) ).longOpt( "file" ).hasArg().desc( "Force the use of an alternate POM file (or directory with pom.xml)" ).build() );
options.addOption( Option.builder( Character.toString( SET_SYSTEM_PROPERTY ) ).numberOfArgs( 2 ).valueSeparator( '=' ).desc( "Define a system property" ).build() );
options.addOption( Option.builder( Character.toString( SET_USER_PROPERTY ) ).numberOfArgs( 2 ).valueSeparator( '=' ).desc( "Define a user property" ).build() );
options.addOption( Option.builder( Character.toString( OFFLINE ) ).longOpt( "offline" ).desc( "Work offline" ).build() );
options.addOption( Option.builder( Character.toString( VERSION ) ).longOpt( "version" ).desc( "Display version information" ).build() );
options.addOption( Option.builder( Character.toString( QUIET ) ).longOpt( "quiet" ).desc( "Quiet output - only show errors" ).build() );
Expand Down
8 changes: 4 additions & 4 deletions maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ private CommandLine cliMerge( CommandLine mavenArgs, CommandLine mavenConfig )
List<Option> setPropertyOptions = new ArrayList<>();
for ( Option opt : mavenArgs.getOptions() )
{
if ( String.valueOf( CLIManager.SET_SYSTEM_PROPERTY ).equals( opt.getOpt() ) )
if ( String.valueOf( CLIManager.SET_USER_PROPERTY ).equals( opt.getOpt() ) )
{
setPropertyOptions.add( opt );
}
Expand All @@ -490,7 +490,7 @@ private CommandLine cliMerge( CommandLine mavenArgs, CommandLine mavenConfig )
{
commandLineBuilder.addOption( opt );
}
// finally add the CLI system properties
// finally add the CLI user properties
for ( Option opt : setPropertyOptions )
{
commandLineBuilder.addOption( opt );
Expand Down Expand Up @@ -1745,7 +1745,7 @@ int calculateDegreeOfConcurrency( String threadConfiguration )
}

// ----------------------------------------------------------------------
// System properties handling
// Properties handling
// ----------------------------------------------------------------------

static void populateProperties( CommandLine commandLine, Properties systemProperties, Properties userProperties )
Expand All @@ -1759,7 +1759,7 @@ static void populateProperties( CommandLine commandLine, Properties systemProper
// ----------------------------------------------------------------------

final Properties userSpecifiedProperties = commandLine.getOptionProperties(
String.valueOf( CLIManager.SET_SYSTEM_PROPERTY ) );
String.valueOf( CLIManager.SET_USER_PROPERTY ) );
userSpecifiedProperties.forEach(
( prop, value ) -> setCliProperty( (String) prop, (String) value, userProperties )
);
Expand Down
6 changes: 3 additions & 3 deletions maven-embedder/src/test/java/org/apache/maven/cli/MavenCliTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,9 +338,9 @@ public void testMVNConfigurationFunkyArguments()
String revision = System.getProperty( "revision" );
assertEquals( "8.2.0", revision );

assertEquals( "bar ", request.getSystemProperties().getProperty( "foo" ) );
assertEquals( "bar two", request.getSystemProperties().getProperty( "foo2" ) );
assertEquals( "Apache Maven", request.getSystemProperties().getProperty( "label" ) );
assertEquals( "bar ", request.getUserProperties().getProperty( "foo" ) );
assertEquals( "bar two", request.getUserProperties().getProperty( "foo2" ) );
assertEquals( "Apache Maven", request.getUserProperties().getProperty( "label" ) );
Comment thread
gnodet marked this conversation as resolved.

assertEquals( "-Dpom.xml", request.getCommandLine().getOptionValue( CLIManager.ALTERNATE_POM_FILE ) );
}
Expand Down
Loading