Do not set user properties as system properties

[gnodet]

No description provided.

[mthmulders]

I think many plugins and frameworks depend on the fact that Maven properties can be read through system properties. If we decide to drop this (undocumented) feature, it would certainly warrant a JIRA ticket that clearly mentions this breaks from previous releases.

[cstamas]

Can we NOT do this for Model v4.0.0 (hence use of Maven3 plugins) and DO THIS for Model 4.1+ maybe? As @mthmulders says, I would bet in some beer there are Maven3 plugins relying on this.

[michael-o]

Can we NOT do this for Model v4.0.0 (hence use of Maven3 plugins) and DO THIS for Model 4.1+ maybe? As @mthmulders says, I would bet in some beer there are Maven3 plugins relying on this.

How is this model related?

[michael-o]

I think many plugins and frameworks depend on the fact that Maven properties can be read through system properties. If we decide to drop this (undocumented) feature, it would certainly warrant a JIRA ticket that clearly mentions this breaks from previous releases.

This is the same as with maven.multimoduleDirectory never documented. What we should do is to us this PR and run a build of all our ITs and see what fails. I spoke about this with @slawekjaranowski last year, Invoker and friends need to be fixed first.

[gnodet]

It may be possible to replace the system properties with a Properties derived class that would log some warnings if accessing Maven user properties from system properties.
However, the question is whether expecting to not use system properties anymore is achievable at all, given there may be lots of third party dependencies relying on those, and sometime with no easy way to work around that.
We do even use some system properties, when configuring logging for example.
So I'm tempted to close that as won't fix and forget about trying to go further on that side.
Thoughts ? @michael-o @cstamas

[cstamas]

I'd not give up fully, maybe "just" postpone for 4.1 or 5.x? With new API we can have "clean slate", a sorta new beginning. For sure that things coming from Maven3 expect user properties pushed to System properties, but for newly done stuff we can avoid that.

[michael-o]

I'd not give up fully, maybe "just" postpone for 4.1 or 5.x? With new API we can have "clean slate", a sorta new beginning. For sure that things coming from Maven3 expect user properties pushed to System properties, but for newly done stuff we can avoid that.

I agree, people have the options to set properties via MAVEN_OPTS. We shouldn't give the delusion that we can do that reliable after the VM has been started.

For starters we can issue a warning when an existing system property has been overwritten.

[cstamas]

For starters we can issue a warning when an existing system property has been overwritten.

This never happens, nor happened ever: the push was always "set property if key not present already". See

maven/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/LookupInvoker.java

Lines 182 to 203 in 3761953

	/**
	* Note: this method is called twice from {@link #doInvoke(LookupContext)} and modifies context. First invocation
	* when {@link LookupContext#pushedUserProperties} is null will push user properties IF key does not already
	* exist among Java System Properties, and collects all they key it pushes. Second invocation happens AFTER
	* {@link PropertyContributor} SPI invocation, and "refreshes" already pushed user properties by re-writing them
	* as SPI may have modified them.
	*/
	protected void pushUserProperties(C context) throws Exception {
	ProtoSession protoSession = context.protoSession;
	HashSet<String> sys = new HashSet<>(protoSession.getSystemProperties().keySet());
	if (context.pushedUserProperties == null) {
	context.pushedUserProperties = new HashSet<>();
	protoSession.getUserProperties().entrySet().stream()
	.filter(k -> !sys.contains(k.getKey()))
	.peek(k -> context.pushedUserProperties.add(k.getKey()))
	.forEach(k -> System.setProperty(k.getKey(), k.getValue()));
	} else {
	protoSession.getUserProperties().entrySet().stream()
	.filter(k -> context.pushedUserProperties.contains(k.getKey()) || !sys.contains(k.getKey()))
	.forEach(k -> System.setProperty(k.getKey(), k.getValue()));
	}
	}

[michael-o]

For starters we can issue a warning when an existing system property has been overwritten.

This never happens, nor happened ever: the push was always "set property if key not present already". See

maven/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/LookupInvoker.java

Lines 182 to 203 in 3761953

	/**
	* Note: this method is called twice from {@link #doInvoke(LookupContext)} and modifies context. First invocation
	* when {@link LookupContext#pushedUserProperties} is null will push user properties IF key does not already
	* exist among Java System Properties, and collects all they key it pushes. Second invocation happens AFTER
	* {@link PropertyContributor} SPI invocation, and "refreshes" already pushed user properties by re-writing them
	* as SPI may have modified them.
	*/
	protected void pushUserProperties(C context) throws Exception {
	ProtoSession protoSession = context.protoSession;
	HashSet<String> sys = new HashSet<>(protoSession.getSystemProperties().keySet());
	if (context.pushedUserProperties == null) {
	context.pushedUserProperties = new HashSet<>();
	protoSession.getUserProperties().entrySet().stream()
	.filter(k -> !sys.contains(k.getKey()))
	.peek(k -> context.pushedUserProperties.add(k.getKey()))
	.forEach(k -> System.setProperty(k.getKey(), k.getValue()));
	} else {
	protoSession.getUserProperties().entrySet().stream()
	.filter(k -> context.pushedUserProperties.contains(k.getKey()) || !sys.contains(k.getKey()))
	.forEach(k -> System.setProperty(k.getKey(), k.getValue()));
	}
	}

In Maven 3 we always pushed, no matter what.

[gnodet]

For starters we can issue a warning when an existing system property has been overwritten.

This never happens, nor happened ever: the push was always "set property if key not present already". See

maven/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/LookupInvoker.java

Lines 182 to 203 in 3761953

	/**
	* Note: this method is called twice from {@link #doInvoke(LookupContext)} and modifies context. First invocation
	* when {@link LookupContext#pushedUserProperties} is null will push user properties IF key does not already
	* exist among Java System Properties, and collects all they key it pushes. Second invocation happens AFTER
	* {@link PropertyContributor} SPI invocation, and "refreshes" already pushed user properties by re-writing them
	* as SPI may have modified them.
	*/
	protected void pushUserProperties(C context) throws Exception {
	ProtoSession protoSession = context.protoSession;
	HashSet<String> sys = new HashSet<>(protoSession.getSystemProperties().keySet());
	if (context.pushedUserProperties == null) {
	context.pushedUserProperties = new HashSet<>();
	protoSession.getUserProperties().entrySet().stream()
	.filter(k -> !sys.contains(k.getKey()))
	.peek(k -> context.pushedUserProperties.add(k.getKey()))
	.forEach(k -> System.setProperty(k.getKey(), k.getValue()));
	} else {
	protoSession.getUserProperties().entrySet().stream()
	.filter(k -> context.pushedUserProperties.contains(k.getKey()) || !sys.contains(k.getKey()))
	.forEach(k -> System.setProperty(k.getKey(), k.getValue()));
	}
	}

In Maven 3 we always pushed, no matter what.

Correct, see

maven/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java

Line 1531 in e7e0cbb

userProperties.forEach((k, v) -> System.setProperty((String) k, (String) v));

This goes back to:
https://github.com/apache/maven/pull/1062/files#diff-b8b814f5b6b3855ae79dc45a0266b94871193dcef42803c316e07409e94af35cR1569-R1571