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

Ensure maven.properties can be forwarded to system properties for exec:java #346

Conversation

rmannibucau
Copy link
Contributor

No description provided.

@slawekjaranowski
Copy link
Member

We shouldn't modify system property at all, it is shared by whole JVM and can have impact on other thread .. and so on. Even more some system properties are immutable like user.dir

I don't know user case why Maven project properties are needed by user class.
In such case we can propose to user to create main method with additional argument like:

void main( String args[], Properties mavenProjectProperties )

@rmannibucau
Copy link
Contributor Author

@slawekjaranowski I can tend to agree but since it is already the case it is just a matter of enabling to access maven properties instead of having to wire 50+ properties in <systemProperty> blocks. I think we should stay consistent with exec plugin configuration style - why I didn't use implementation for ex and the issue with using new injections like you propose is that you will quickly need to inject MavenProject etc. I would be happy to work on that in another PR but right now I need a main to be reusable in maven and standalone and not rewrite the 50+ props in maven case - pom already contains most of the needed properties for another plugin using the same set of props - so hope we can get this feature onboard first and then propose a new better programming model but both are not the exact same one (even if main(args, System.getProperties()) could be a woakround if you can rewrite your main, ie it is not packaged in a lib).

@slawekjaranowski
Copy link
Member

For me ExecJavaMojo is not threadSafe if we override system properties. Good be to consider such ...

@rmannibucau
Copy link
Contributor Author

@slawekjaranowski sure ExecJavaMojo is not thread safe and before this PR it was even leaking the system properties after mojo execution (which is worse cause you are not even able to chain executions in a single threaded build without side effects as of today). This last one was fixed because with this new feature we would leak way too much between executions.
There are several options to enhance the thread safety but they need some maven setup - like setting system properties with a custom Properties suclass to enable to switch the thread (local) vision for some executions etc...
This PR is really just about enabling to forward all props at once to give maven context to a reusable main but really agree we can work on something better on the long run when user writes its own main.

Hope it makes sense.

@rmannibucau
Copy link
Contributor Author

Can we get it merged 🙏 ?

@slawekjaranowski do you want I work on your signature proposal after the merge or do we wait the need arises - I'm fine with both options?

@slawekjaranowski
Copy link
Member

Can we get it merged 🙏 ?

@slawekjaranowski do you want I work on your signature proposal after the merge or do we wait the need arises - I'm fine with both options?

you can merge - as I already approved

@rmannibucau
Copy link
Contributor Author

@slawekjaranowski no perms to do it (not sure it is an issue with my github account or anything else)

@slawekjaranowski slawekjaranowski merged commit 95bff1c into mojohaus:master Nov 21, 2022
@slawekjaranowski
Copy link
Member

eh... 5 commit merged 😠 forgot to squash

@rmannibucau
Copy link
Contributor Author

@slawekjaranowski want to revert/reapply? Guess we can

@slawekjaranowski
Copy link
Member

Never mind - it is ok, only some more commits then one.

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