-
Notifications
You must be signed in to change notification settings - Fork 101
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
Fix for polyglot when in lib/ext #319
Conversation
Originally polyglot worked when loaded via .mvn/extensions.xml but not when created custom distro (put in lib/ext). Reason was simple, as Maven core beans were not overridden when in "flat space", that causes lib/ext, unlike mvn extensions.
@headius This PR is needed to make polyglot work when created "custom Maven distro" (repackaged Maven with polyglot in lib/ext). I tested and figured it does not work with current releases. Tested with polyglot ruby, but this is polyglot-common change, not lang specific at all. |
@mkristian ping as well ^^ so my advice, creating "custom maven distro" (unpack mvn zip, put polyglot-ruby w/ deps into lib/ext, repack) will NOT work with current version (tested manually). This PR adds the change that makes it work, but also keeps original capability to work from .mvn/extensions.xml as well. |
We may want 0.7.1 release with this change. |
@cstamas Thanks for the ping on this one. I tried to integrate polyglot into our ruby-maven gem myself and thought I must be doing something wrong! Glad to hear that it's a pretty simple fix in polyglot. Let me know if you need my help getting this integrated and released (testing etc). I will open a bug or reference this PR from the ruby-maven side so folks know we are moving forward with a solution. |
I can merge and do 0.7.1 asap. Ok? |
If you are happy with it, go ahead with release. I can work with @mkristian this week/end to get it integrated into ruby-maven and let you know if we have any further issues. |
https://github.com/takari/polyglot-maven/releases/tag/polyglot-0.7.1 Sync to central pending... |
@cstamas cool.thanks. yes I am going to work on this integration this weekend. |
This change now likely breaks the whole
|
Good point :-D Lets see if it helps: |
Now it fails with
|
As you invoke modelProcessor with Which in case polyglot is installed is invalid. Polyglot is envisioned to be "installed" or "not installed" (is Maven extension), and when installed it "takes over some key components". Am unsure what this "pomless tycho extras" is doing and why it uses Polyglot at all... |
Where is this documented? It obviously has worked in 0.7.0 (and previous versions) but now fails with 0.7.1
Tycho is a Maven extension as well and uses (and extends) polyglot since years and actually was the reason for changes/bugfixes I proposed here over the time
Tycho extends Polyglot in a way that it can use Eclipse Metadata files as build input (like yaml or json or ... in this repository): https://tycho.eclipseprojects.io/doc/latest/StructuredBuild.html |
Beside that, the javadoc clearly states that null is allowed, so this is a bug of polyglot-common.
|
Passing an empty map results in
|
There's not many code changes, why is that broken ? Maybe you should raise a different issue. |
I can only tell that it worked in 0.7.0 but fails in 0.7.1 and this one looks like it is changing some fundamental things I have now created |
@cstamas @mkristian I am just checking in to see how we're coming with this work and the integration into JRuby's ruby-maven gem. Let me know how I can help! |
@laeubi this issue is close, can you open a new one ? Is there a fix available yet ? |
So the timeline of events:
|
So, for JRuby: @headius @mkristian I think all your issues are resolved, and please go for Maven 3.9.9 as then you can even leave your docker images unchanged, as Maven 3.9.9 (but not any before!) will pick up .mvn/extensions.xml from FS root. For Tycho: again, am unsure how and for what polyglot is used in there and how it worked before, given the problem is that polyglot became "active" while it was not before. So why is it in classpath then in the first place? |
Tycho is providing a polyglot implementation "since ever" (oldest one I can found is form 2015) like polyglot-yaml, or polyglot-ruby, or ... just that is is not part of the polyglot repo and released independent.
I never said that anywhere so I'm confused, polyglot is and was always active for Tycho (using so whatever I pass ( One special thing to consider is, that polyglot has these options:
I can only guess that the second case is broken now somehow or there is a different ordering of calls or a missing delegation call as the message says
or you get a NPE
so it seems the modelreader expects a custom model in all cases. |
It's a bit unfortunate, that this never came to the attention of the polyglot maintainers. It would be good, if we add Tycho to the list polyglot dialects, even if it is maintained externally. |
I try to finalise this today or tomorrow with with done by @cstamas
|
That is the change how we use all the new stuff from here: jruby/ruby-maven#11 @headius there are still errors in the test which I just discovered this minute. That was totally under my radar. |
Fix found #326 |
Originally polyglot worked when loaded via .mvn/extensions.xml but not when created custom distro (put in lib/ext).
Reason was simple, as Maven core beans were not overridden when in "flat space", that causes lib/ext, unlike mvn extensions.
To make it work from lib/ext (when components are placed into maven core), following is needed: