Skip to content
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.

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

[MINVOKER-364] Rename invoker.systemPropertiesFile to invoker.userPropertiesFile #235

Merged
merged 1 commit into from
May 4, 2024

Conversation

slawekjaranowski
Copy link
Member

@olamy
Copy link
Member

olamy commented May 3, 2024

not sure I understand this naming change,
At the end of the day, those properties will be system properties used to fork the Maven invocation.
So they are the system properties of the Maven invocation you want to run.
you are confusing with Maven core naming but for an end user POV they are system properties. Those one you set using -D... when using the command line.

@slawekjaranowski
Copy link
Member Author

exactly it are a user properties passed by -D Maven argument
we have:

MAVEN_OPTS='-DsysProperty=xxx' mvn ... -DuserProperty=xxx

so userProperty it is a user property pass to Maven as cli argument

We cen set a system property by MAVEN_OPTS environment variable

@michael-o
Copy link
Member

not sure I understand this naming change, At the end of the day, those properties will be system properties used to fork the Maven invocation. So they are the system properties of the Maven invocation you want to run. you are confusing with Maven core naming but for an end user POV they are system properties. Those one you set using -D... when using the command line.

This has been a mess created long time ago. We need to clean up. Maven should use for itself user props only. If the JVM requires sys props they need to be set before Maven invocation. System.setProperty() cannot guarantee that the actual property is picked up by the receiver. Some read only at startup.

Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

Good -- needs another pair of eyes though.

@slawekjaranowski slawekjaranowski merged commit a741ea0 into master May 4, 2024
50 checks passed
@slawekjaranowski slawekjaranowski deleted the MINVOKER-364 branch May 4, 2024 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants