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

Decouple Maven Dependencies from JRE. #24

Closed
solf opened this issue Sep 29, 2017 · 17 comments
Closed

Decouple Maven Dependencies from JRE. #24

solf opened this issue Sep 29, 2017 · 17 comments

Comments

@solf
Copy link

solf commented Sep 29, 2017

Please decouple setting external annotations for 'Maven Dependencies' from JRE (specifically when using m2e.jdt.annotationpath).

I've been using my own hacked version of m2e.jdt that sets external annotations for 'Maven Dependencies' only (one global folder for all Maven dependencies in all projects). For JRE I'm relying on ability to specify external annotations for rt.jar. In my case those point to different directories (it seems prudent to have separate directory for each JDK version).

I'd rather get rid of my hack in favor of something less hacky -- I tried your extension, but when using it (with m2e.jdt.annotationpath) it sets external annotations for JRE thus breaking my settings for rt.jar.

@vorburger
Copy link
Member

@solf thanks for your interest in this project.

This problem is specific to the use of m2e.jdt.annotationpath, right? FYI personally I'm now only using the other approach, with the EEA that are getting contributed to https://github.com/lastnpe/eclipse-null-eea-augments... and not this old global single one, anymore.

But we certainly anyway still have two new properties, separating Maven and JRE EEA. (For backwards compatibility for any other existing users, IMHO it's probably best not to change m2e.jdt.annotationpath, but have new large ones, something like say m2e.maven.annotationpath and m2e.jre.annotationpath.)

Myself I will unfortunately not have the time to implement this for you, but should have any interest in contributing a pull request for this, we'd certainly look at it.

@solf
Copy link
Author

solf commented Sep 30, 2017

Thanks for the update.
I'll see when I have time to figure out how to do those infamous pull requests :)

@maggu2810
Copy link
Contributor

@solf @vorburger Any news about this?
Currently I am separating the annotations for JRE and Maven dependencies in my .classpath file:

	<classpathentry kind="con" path="org.eclipse.m2e.MAVEN2_CLASSPATH_CONTAINER">
		<attributes>
			<attribute name="maven.pomderived" value="true"/>
			<attribute name="annotationpath" value="/eclipse-external-annotations/others"/>
		</attributes>
	</classpathentry>
	<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-1.8">
		<attributes>
			<attribute name="maven.pomderived" value="true"/>
			<attribute name="annotationpath" value="/eclipse-external-annotations/jre-8"/>
		</attributes>
	</classpathentry>

This information is every time lost the Eclipse IDE trigger a Maven update project...

Very annoying.

For a multi-module project I would prefer to define that propertes (e.g. m2e.maven.annotationpath and m2e.jre.annotationpath) in the base POM and it should be considered by every artifact of the reactor.

@solf
Copy link
Author

solf commented Nov 9, 2017

I am not currently looking at this as my hacked jar works fine for my needs.
If there's public interest, I might try to do this, but I'm totally unsure how the whole 'pull request' system works around here.

@maggu2810
Copy link
Contributor

If you already have the changed sources, you could push it to your local repo and give me the branch name. Then I could have a look at.

@solf
Copy link
Author

solf commented Nov 9, 2017

You're assuming that I'm using github and/or git :) I do not :)
Also my 'hack' is entirely separate from your plugin -- I wrote it several years ago for my own needs.
But like I said, if there's public interest, I might look into contributing changes to your plugin.

@maggu2810
Copy link
Contributor

You're assuming that I'm using github and/or git :) I do not :)

I assumed you are familiar with Git but not with Github. 😉

But like I said, if there's public interest, I might look into contributing changes to your plugin.

Would be great.

@vorburger
Copy link
Member

@maggu2810 re. "Any news about this?" this issue is about a problem which personally I don't have (I'm now only using the other approach and not this old global single one, anymore) so no plans to implement this myself.. but should you have any interest in contributing a pull request for this, I'd happy to merge it ASAP!

@maggu2810
Copy link
Contributor

maggu2810 commented Nov 13, 2017

@solf At least for me there is no need to contribute it, I have done the necessary changes myself.

@vorburger Is there a setup file for the Eclipse Installer for that project? I have done the changes with my IDE settings but there has been some automatic changes by my save actions. The most changes have been done because of added final keywords to method arguments.
The code change itself (after the formatting changes which I used a separate commit for) are this one: https://github.com/maggu2810/eclipse-external-annotations-m2e-plugin/commit/b4b08b680160e09a4699b9b1622cdeab97391bbd

@vorburger
Copy link
Member

@maggu2810 if you have any interest in getting https://github.com/maggu2810/eclipse-external-annotations-m2e-plugin/commit/b4b08b680160e09a4699b9b1622cdeab97391bbd to be integrated here, can I let you raise a PR? (Full disclosure: From my side I don't "need" this at all, but if you have any interest in having this merged, feel free to raise the PR for review.)

@maggu2810
Copy link
Contributor

That has been my intention and so I asked you if there is a setup file for the Eclipse installer to fit the format of the source code to this project's setting.
If I create a PR with my changes there will be two commits and the first one changes some format and adds final (as written above).
So my question about the project guidelines...
😉

@solf
Copy link
Author

solf commented Nov 13, 2017

Hi, since you're dealing with this -- may I ask that you consider an option to just set external annotations for Maven container once per workspace and don't have to bother specifying it on per-project basis? Depending on what the code is doing, it might actually be already included if you specify -D option in eclipse.ini (I believe 'standard' Maven uses -D options as its variables) but I'm unsure of the interactions with this plugin.

Ideally one should be able to specify 'workspace default' and then be able to override it on per-project as needed.

@vorburger
Copy link
Member

@maggu2810 oh! Sorry, perhaps I should read comments completely in the future? 😃

To answer, this project is really just 1 class ;-) so there is Oomph Setup model nor any guidelines...

@sylvainlaurent FYI this one may interest you - just because this of course was originally your Plugin!

@sylvainlaurent
Copy link
Member

sylvainlaurent commented Nov 15, 2017

@solf : I don't think it's possible to set external annotation for the Maven container at the workspace level, I don't see the possibility in eclipse preferences.
It seems possible for the JRE however. But is it worth doing it if you don't have the maven container ?
My own experience with maven and IDEs is to prefer having such settings in the maven projects, usually factored in a parent pom.xml common to multiple projects. This reduces dependencies on developer-specific settings.

@solf
Copy link
Author

solf commented Nov 16, 2017

@sylvainlaurent : I'm not sure I understand your question. Yes, it is not possible to set up external annotations for Maven using standard Eclipse -- hence the need for existence of this plugin (or my own hack that I've been using).

My concern right now is that right now I have no need to specify different external Eclipse annotation locations on a per-project basis (because I share these annotations across all projects -- it makes sense as they are not project-specific).

So I'm asking @maggu2810 to consider providing an option in his implementation to just use one value (set somewhere) for all projects. Depending on how the code is implemented, it might already work this way if you specify -D option in eclipse.ini (because Maven uses system properties as its own variables).

@vorburger
Copy link
Member

@solf to pick up on this comment from you #26 (comment) here:

I don't think we want to use a Java System property from eclipse.ini to configure this plugin - that would be very un-Eclipse. If anything, then perhaps a global workspace wide instead of Maven POM property.

We can only encourage you to learn more about Git and GitHub, if you would like to make a contribution. Here are some pointers you may find useful for self learning:

vorburger pushed a commit that referenced this issue Nov 27, 2017
Fixes: #24
Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
@solf
Copy link
Author

solf commented Nov 27, 2017

It might be 'very un-Eclipse' as you say, but it would totally be in line with how Maven is supposed to operate. Maven picks up -D properties as its own when executing. Since the plugin is using Maven properties for configuring external annotation path, I, for one, would expect -D options to work.

However they don't by default -- I'm not sure why, perhaps Maven integration in Eclipse ignores them or somewhere in there there's a separate JVM or something.

Thanks for the references to the guides by the way, they look helpful. However I don't suppose there's any point in creating pull request I proposed since you seem to be opposed to the idea in principle, is there?

I'm not opposed to creating global configuration option instead, however it's not something I'm familiar with (or any plugin development really), so I don't see myself doing that right now. For my own use I'm already using the modification I proposed and it seems to work as expected -- so thanks a lot to everyone involved for making this possible!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants