To add to the topic I think that if we fix/change the class loading, we shouldn't do that in a bug fix release. I'd rather see a 3.6.0 with it thought through, fixed and documented (could be in ITs). The arguing being that a bug fix release shouldn't cause new issues in plugins etc.
/Anders On Sun, Sep 24, 2017 at 8:28 PM, Stephen Connolly < stephen.alan.conno...@gmail.com> wrote: > I wonder should we do a hangout to decide what you do? > > What times on Monday work best? > > I can maybe do 8:30-9:30pm Irish time > > https://www.timeanddate.com/worldclock/meetingdetails. > html?year=2017&month=9&day=25&hour=19&min=30&sec=0&p1=78&p2=37&p3=179 > > But we’d need to decide who we need and an actual agenda. > > If Monday is too soon I can see if I have a window later this week > > On Sun 24 Sep 2017 at 18:58, Igor Fedorenko <i...@ifedorenko.com> wrote: > > > See my answers/comments inline > > > > > > On Sun, Sep 24, 2017, at 12:07 PM, Stephen Connolly wrote: > > > https://maven.apache.org/guides/mini/guide-maven-classloading.html > says: > > > > > > > When a build plugin is executed, the thread's context classloader is > > set > > > to the plugin classloader. > > > > > > So we'll need to fix something somewhere... > > > > > > > > https://svn.apache.org/repos/infra/websites/production/ > maven/content/reference/maven-classloading.html > > > is unaccessible from the website due to a rewrite rule... > > > > > > Things that seem to be missing: > > > > > > * What is the desired classloading for a plugin that is marked as an > > > extension? Can a plugin have a META-INF/maven/extension.xml to allow > > > exporting classes and artifacts when used as an extension? How should > the > > > classloading look for such a strange beast. > > > > To me, the key requirement is that @Singleton components and class > > static members are singletons when injected in Maven core or in @Mojos. > > This implies that there should be single classloader representing an > > extensions plugins (MNG-5742). > > > > META-INF/maven/extension.xml declares what packages of the extension > > plugin are visible to other (non extension) plugins. > > META-INF/maven/extension.xml does not affect classloading of the > > extension plugin nor it affects the "shape" of other classloaders. > > > > > * How does one access the plugin classloader if we want TCCL to be > other > > > than that, is it a Dependency Injection or something else? > > > > this.getClass().getClassLoader() is the most direct way to access plugin > > classloader. Why do you think we need anything more elaborate? > > > > > > > * What differentiates a Core extension from a Build extension (is it > that > > > a > > > build extension lacks a META-INF/maven/extension.xml and was only > > > declared > > > in the pom.xml, while a core extension either has a > > > META-INF/maven/extension.xml - if declared in the pom - or is an > > > extension > > > declared in .mvn/extensions.xml) > > > > Core extensions are loaded *before* build starts, so they can contribute > > AbstractMavenLifecycleParticipant#afterSessionStart, for example. They > > can also export packages visible to all build plugins, including > > extensions=true. On the flip side, each core extension is effectively > > singleton, you can't have two different versions of the same Core > > extension. Core extensions also have direct access to Maven core classes > > and can do more interesting things there (for better or worse). > > > > Build extensions are part of the project build and as such are limited > > what components they can contribute to the Core and what core classes > > they have access to. > > > > I tried to capture this in the diagram I drew for > > http://takari.io/book/91-maven-classloading.html. > > > > > At this point in time I think we are nearing the point where I may have > > > to > > > declare 3.5.1 abandoned as I think the classloading in that is a > symptom > > > of > > > too many cooks all changing things in different directions. We need a > > > consistent vision of where we want things to go and - while we need not > > > get > > > there in one go - the path presented for others to see. > > > > There were two classloading changes in 3.5.1, namely extensions=true > > plugins now have project realm as TCCL and all realms now use > > application classloader as the parent. Apart from lacking documentation, > > what practical problems have been caused by these two changes? > > > > > > > > Things I think we should consider: > > > > > > 1. Do we want to formally deprecate Build Extensions and the > > > /project/build/extensions element (start logging warnings, etc)? > > > 2. Do we want to formally deprecate plugins as extensions and start > > > logging > > > warnings for > > > > > /project/build/(pluginManagement|.)/plugins/plugin/extensions[text()== > true] > > > > I'd keep them both, and maybe fix/remove maven2-compat codepath. If I > > had to choose between the two, however, I'd choose <plugin> with > > extensions=true. Think of a custom packaging type with mojos the user > > wants to configure in pom.xml, it'd be more tedious to configure if I > > had to add build/extension and build/plugin. > > > > > 3. What is the difference in classloading for a > /project/build/extensions > > > which has a META-INF/maven/extension.xml and one that doesn't? > > > > I think extensions with META-INF/maven/extension.xml should not go > > through maven2-compat codepath. In other words, we need to change the > > current behaviour. > > > > Extensions without META-INF/maven/extension.xml... I am not sure. > > Probably safer to keep the current maven2-compat behaviour. > > > > > I'm keeping the 3.5.1 release in staging until we get a clear vision > for > > > how we want to have classloading so that I can assess whether the 3.5.1 > > > actuality is only moving nearer to the vision (ok to release) or has > > > moved > > > nearer in some ways but further in others (not ok to release) > > > > > > > > > -- > > Regards, > > Igor > > > > > > > > > On 20 September 2017 at 12:44, Igor Fedorenko <i...@ifedorenko.com> > > > wrote: > > > > > > > Real-world scm or wagon <extensions> won't trigger maven2-compat code > > > > path [1]. To avoid that obscure code path we can either make the test > > > > more elaborate (i.e. add dependencies to extjar1/extjar2) or we can > use > > > > extensions <plugin>. Either way I don't think we should spend time on > > > > the code path unlikely to be used in real life. > > > > > > > > [1] > > > > https://github.com/apache/maven/blob/maven-3.5.1/maven- > > > > > > core/src/main/java/org/apache/maven/project/ > DefaultProjectBuildingHelper. > > > > java#L210-L219 > > > > > > > > -- > > > > Regards, > > > > Igor > > > > > > > > On Wed, Sep 20, 2017, at 03:29 AM, Robert Scholte wrote: > > > > > On Wed, 20 Sep 2017 09:12:47 +0200, Stephen Connolly > > > > > <stephen.alan.conno...@gmail.com> wrote: > > > > > > > > > > > On Wed 20 Sep 2017 at 01:29, Igor Fedorenko <i...@ifedorenko.com > > > > > > wrote: > > > > > > > > > > > >> In that case, can I suggest couple of changes to the test > project > > > > > >> > > > > > >> * I thinks it makes more sense to configure extjar1 and extjar2 > as > > > > > >> extensions <plugin> elements in probleN pom.xml files. First, > > there is > > > > > >> no meaningful order between <extensions> and <plugins> elements. > > More > > > > > >> importantly, though, simple <extensions> are treated in special > > > > > >> maven2-compat mode and are not representative of likely > real-world > > > > > >> extensions. > > > > > > > > > > > > > > > > Not sure I agree with this. I think there are jars worth sharing > > across > > > > > multiple plugins, but where making the plugin an extension is a bit > > > > > weird. > > > > > I'm thinking of scm and wagon in this case. > > > > > > > > > > > > > > > > > That sounds like we need documentation updated then. None of that > > is > > > > > > obvious to me. > > > > > > > > > > > > > > > > > >> > > > > > >> * I think we should introduce META-INF/maven/extension.xml to > the > > test > > > > > >> extensions. This metadata what introduced to configure classpath > > > > > >> visibility, so lets use it. > > > > > > > > > > > > > > > > > > Again, not obvious to me, if that file allows control of > classpath > > > > > > visibility then it may be that the only issue *with* 3.5.1 is the > > lack > > > > of > > > > > > documentation... now previous versions would have been adding > > breaking > > > > > > changes from my PoV but that is the past and should not affect > the > > > > 3.5.1 > > > > > > release. > > > > > > > > > > > > PRs for the probe project welcome. I am happy to try and write > docs > > > > once > > > > > > I > > > > > > have an understanding of what the expected behaviours are > > > > > > > > > > > > > > > > > >> > > > > > >> -- > > > > > >> Regards, > > > > > >> Igor > > > > > >> > > > > > >> On Tue, Sep 19, 2017, at 05:12 PM, Stephen Connolly wrote: > > > > > >> > Yes, the expectations are key. Depending on what they are we > may > > > > > >> either > > > > > >> > drop 3.5.1 or go ahead as it depends on whether this is more > > correct > > > > > >> than > > > > > >> > 3.5.0 or swapping one fix for a bug > > > > > >> > > > > > > >> > On Tue 19 Sep 2017 at 21:39, Igor Fedorenko < > > i...@ifedorenko.com> > > > > > >> wrote: > > > > > >> > > > > > > >> > > Just to confirm I understand what we are trying to establish > > here. > > > > > >> We > > > > > >> > > want to decide the expected/desired component injection > > behaviour > > > > > >> and > > > > > >> > > classpath visibility in the absence of package and artifact > > export > > > > > >> > > configuration (i.e. META-INF/maven/extension.xml file). Did > I > > get > > > > > >> this > > > > > >> > > right? > > > > > >> > > > > > > > >> > > -- > > > > > >> > > Regards, > > > > > >> > > Igor > > > > > >> > > > > > > > >> > > On Tue, Sep 19, 2017, at 03:52 PM, Robert Scholte wrote: > > > > > >> > > > Let's do it like this: > > > > > >> > > > > > > > > >> > > > > > > > >> https://cwiki.apache.org/confluence/download/ > attachments/2329841/ > > > > classrealms.pdf?api=v2 > > > > > >> > > > > > > > > >> > > > Robert > > > > > >> > > > > > > > > >> > > > On Tue, 19 Sep 2017 21:08:39 +0200, Stephen Connolly > > > > > >> > > > <stephen.alan.conno...@gmail.com> wrote: > > > > > >> > > > > > > > > >> > > > > I think you will need a link to the PDF as attachments > are > > > > > >> stripped > > > > > >> > > from > > > > > >> > > > > the ML > > > > > >> > > > > > > > > > >> > > > > On Tue 19 Sep 2017 at 19:57, Robert Scholte > > > > > >> <rfscho...@apache.org> > > > > > >> > > wrote: > > > > > >> > > > > > > > > > >> > > > >> Attached a single page overview. > > > > > >> > > > >> > > > > > >> > > > >> Per block you'll see in the upper left corner the > > executed > > > > > >> plugin > > > > > >> > > > >> The left column contains the extensions and plugin in > > orderas > > > > > >> > > specified > > > > > >> > > > >> in > > > > > >> > > > >> the pom.xml > > > > > >> > > > >> In every classloadercolumn you'll see numbers which > > represent > > > > > >> the > > > > > >> > > order. > > > > > >> > > > >> > > > > > >> > > > >> I hope I didn't make any mistakes. > > > > > >> > > > >> Tomorrow I have enough time to see if I understand > what's > > > > > >> happening > > > > > >> > > > >> here. > > > > > >> > > > >> > > > > > >> > > > >> I will come back with my conclusions. > > > > > >> > > > >> > > > > > >> > > > >> Robert > > > > > >> > > > >> > > > > > >> > > > >> On Tue, 19 Sep 2017 06:55:08 +0200, Igor Fedorenko < > > > > > >> > > i...@ifedorenko.com> > > > > > >> > > > >> wrote: > > > > > >> > > > >> > > > > > >> > > > >> > TL;DR your test project exposed two existing bugs, > one > > > > > >> change in > > > > > >> > > > >> > behaviour and one quirk I can't explain > > > > > >> > > > >> > > > > > > >> > > > >> > * Build `<extensions>` are loaded by two > classloaders, > > > > which > > > > > >> is > > > > > >> a > > > > > >> > > bug > > > > > >> > > > >> in > > > > > >> > > > >> > DefaultProjectBuildingHelper#createProjectRealm and > > > > explains > > > > > >> why you > > > > > >> > > > >> see > > > > > >> > > > >> > extjar1/extjar2 in the output > > > > > >> > > > >> > * ClassRealm does not allow same foreign-import from > > > > multiple > > > > > >> > > > >> > classloaders, which is a bug and explains why it is > not > > > > > >> possible to > > > > > >> > > > >> load > > > > > >> > > > >> > same resource from multiple plugins/extensions > > > > > >> > > > >> > * TCCL does not have access to private (i.e. not > > exported) > > > > > >> resources > > > > > >> > > > >> of > > > > > >> > > > >> > this extensions plugin, which is a change of > behaviour > > > > > >> introduced by > > > > > >> > > > >> > mng-6209 fix > > > > > >> > > > >> > * Also, component injection order appears to be > > backwards, > > > > > >> but > > > > > >> maybe > > > > > >> > > > >> > Stuart can explain why. > > > > > >> > > > >> > > > > > > >> > > > >> > > > > > > >> > > > >> > Below is more detailed explanation of expected and > > observed > > > > > >> > > behaviour > > > > > >> > > > >> > > > > > > >> > > > >> > > > > > > >> > > > >> > ## Component injection depends on the currently > running > > > > > >> plugin > > > > > >> and > > > > > >> > > the > > > > > >> > > > >> > injection site > > > > > >> > > > >> > > > > > > >> > > > >> > Currently running plugins have access to the > following > > > > > >> component > > > > > >> > > > >> > implementations: > > > > > >> > > > >> > > > > > > >> > > > >> > * Regular plugin has access to components implemented > > by > > > > the > > > > > >> plugin, > > > > > >> > > > >> > project build extensions, if any (via project class > > realm > > > > > >> foreign > > > > > >> > > > >> > import) and Maven Core. > > > > > >> > > > >> > * Extension plugin has access to components > > implemented by > > > > > >> the > > > > > >> > > project > > > > > >> > > > >> > build extensions and Maven Core. > > > > > >> > > > >> > * Without a running plugin (e.g., during project > > dependency > > > > > >> > > > >> resolution), > > > > > >> > > > >> > components implemented by the project build > extensions > > and > > > > > >> Maven > > > > > >> > > Core > > > > > >> > > > >> > are accessible. > > > > > >> > > > >> > > > > > > >> > > > >> > Different injection sites have access to the > following > > > > > >> component > > > > > >> > > > >> > interfaces: > > > > > >> > > > >> > > > > > > >> > > > >> > * Maven Core has access to component interfaces > > defined by > > > > > >> the > > > > > >> core > > > > > >> > > > >> > itself (obviously) > > > > > >> > > > >> > * Project build extensions have access to **public** > > > > > >> component > > > > > >> > > > >> > interfaces defined by Maven Core and component > > interfaces > > > > > >> defined by > > > > > >> > > > >> the > > > > > >> > > > >> > build extension itself (there is no way to access > > component > > > > > >> > > interfaces > > > > > >> > > > >> > defined in other extensions) > > > > > >> > > > >> > * Regular plugins have access to **public** component > > > > > >> interfaces > > > > > >> > > > >> defined > > > > > >> > > > >> > by Maven Core, component interfaces **exported** by > > build > > > > > >> extensions > > > > > >> > > > >> and > > > > > >> > > > >> > component interfaces defined in the plugin itself > > > > > >> > > > >> > > > > > > >> > > > >> > For injection to work, injection site has to have > > access to > > > > > >> the > > > > > >> > > > >> > component interface and the component implementation > > must > > > > be > > > > > >> > > > >> accessible > > > > > >> > > > >> > through the current context. > > > > > >> > > > >> > > > > > > >> > > > >> > From what I can tell, in your example all plugins > have > > > > access > > > > > >> to the > > > > > >> > > > >> > right components when using current 3.5.2-SNAPSHOT. > The > > > > > >> injection > > > > > >> > > > >> order > > > > > >> > > > >> > does appear to be backwards from what I expected, > > however. > > > > > >> > > > >> > > > > > > >> > > > >> > > > > > > >> > > > >> > ## Resources lookup fully depends on classpath > > visibility, > > > > > >> > > > >> specifically > > > > > >> > > > >> > > > > > > >> > > > >> > * Regular plugin class realm has access to resources > > from > > > > the > > > > > >> plugin > > > > > >> > > > >> > itself, from **exported** packages of the project > build > > > > > >> extensions > > > > > >> > > and > > > > > >> > > > >> > **public** Maven Core packages > > > > > >> > > > >> > * Extensions plugin class realm has access to the > > resources > > > > > >> from the > > > > > >> > > > >> > extensions plugin itself and from **public** Maven > Core > > > > > >> packages > > > > > >> > > > >> > * Project class realm has access to classes and > > resources > > > > > >> > > **exported** > > > > > >> > > > >> > by project build extensions and **public** Maven Core > > > > > >> packages > > > > > >> > > > >> > > > > > > >> > > > >> > I see three problems here > > > > > >> > > > >> > > > > > > >> > > > >> > * Maven adds build single-jar `<extensions>` elements > > > > > >> directly > > > > > >> to > > > > > >> > > > >> > project class realm **and** creates separate > extensions > > > > class > > > > > >> realms > > > > > >> > > > >> for > > > > > >> > > > >> > them. Which results in duplicate classes/resources > > loaded > > > > by > > > > > >> two > > > > > >> > > > >> > classloaders and explains why you see extjar1/extjar2 > > > > output > > > > > >> (which > > > > > >> > > > >> you > > > > > >> > > > >> > shouldn't according to the explanation above) > > > > > >> > > > >> > * ClassRealm does not allow foreign-import of the > same > > > > > >> package > > > > > >> from > > > > > >> > > > >> > multiple classloaders. This makes it impossible to > > load the > > > > > >> same > > > > > >> > > > >> > resource from multiple plugins/extensions. > > > > > >> > > > >> > * Extensions plugins cannot access their own private > > (i.e. > > > > > >> not > > > > > >> > > > >> exported) > > > > > >> > > > >> > resources via TCCL, this is change in behaviour > > introduced > > > > by > > > > > >> > > mng-6209 > > > > > >> > > > >> > fix > > > > > >> > > > >> > > > > > > >> > > > >> > Hope this helps > > > > > >> > > > >> > > > > > >> > > > >> > > > > > >> > > --------------------------------------------------------------------- > > > > > >> > > > >> To unsubscribe, e-mail: dev-unsubscribe@maven.apache. > org > > > > > >> > > > >> For additional commands, e-mail: > > dev-h...@maven.apache.org > > > > > >> > > > > > > > > >> > > > > > > > > >> > > --------------------------------------------------------------------- > > > > > >> > > > To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org > > > > > >> > > > For additional commands, e-mail: > dev-h...@maven.apache.org > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > >> > > --------------------------------------------------------------------- > > > > > >> > > To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org > > > > > >> > > For additional commands, e-mail: dev-h...@maven.apache.org > > > > > >> > > > > > > > >> > > -- > > > > > >> > Sent from my phone > > > > > >> > > > > > >> > > --------------------------------------------------------------------- > > > > > >> To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org > > > > > >> For additional commands, e-mail: dev-h...@maven.apache.org > > > > > >> > > > > > >> -- > > > > > > Sent from my phone > > > > > > > > > > ------------------------------------------------------------ > --------- > > > > > To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org > > > > > For additional commands, e-mail: dev-h...@maven.apache.org > > > > > > > > > > > > > ------------------------------------------------------------ > --------- > > > > To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org > > > > For additional commands, e-mail: dev-h...@maven.apache.org > > > > > > > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org > > For additional commands, e-mail: dev-h...@maven.apache.org > > > > -- > Sent from my phone >
