Le dim. 12 nov. 2023 à 10:44, Niels Basjes <[email protected]> a écrit :
> Hi, > > > Looking quickly the drop of the conditional read of the manifest seems > the > > issue for tests. > > Yes it was. Must have been late when I wrote that ... > I have fixed this silly error of mine and now it all works. > > > In terms of impl my 2cts would be that we shouldnt have had jdependency > in > > this plugin IMHO so better to stay off it (in particular cause there it > is > > really not needed, probably saner to read the manifest first then if mjar > > is enabled list META-INF/$digit/.* and enable to copy them at need but > > otherwise impl looks at the right location for me. > > The current implementation of shade already uses this dependency. > I just added a lacking feature which makes all of the JEP238 classes > available in the list of classes. > Yep, old laziness I'd say but has pitfalls. Anyway, propagating its usage is bad IMHO (in particular for mjar where it is not needed and would couple us more to an unneeded dep, plus preloading overriden classes is faster than doing it one by one in general). > Niels > > On Sat, Nov 11, 2023 at 7:47 PM Romain Manni-Bucau <[email protected]> > wrote: > > > Hi Niels, > > > > Looking quickly the drop of the conditional read of the manifest seems > the > > issue for tests. > > > > In terms of impl my 2cts would be that we shouldnt have had jdependency > in > > this plugin IMHO so better to stay off it (in particular cause there it > is > > really not needed, probably saner to read the manifest first then if mjar > > is enabled list META-INF/$digit/.* and enable to copy them at need but > > otherwise impl looks at the right location for me. > > > > Romain Manni-Bucau > > @rmannibucau <https://twitter.com/rmannibucau> | Blog > > <https://rmannibucau.metawerx.net/> | Old Blog > > <http://rmannibucau.wordpress.com> | Github < > > https://github.com/rmannibucau> | > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book > > < > > > https://www.packtpub.com/application-development/java-ee-8-high-performance > > > > > > > > > Le sam. 11 nov. 2023 à 19:29, Niels Basjes <[email protected]> a écrit : > > > > > Hi, > > > > > > I've been working on trying to make the shade plugin support jars that > > > are Multi Release Jars (JEP-238) > > > https://issues.apache.org/jira/browse/MSHADE-406 > > > A Multi Release Jar simply means you can have a single class in a > single > > > jar with multiple implementations (and thus multiple class files in the > > > jar) which are used depending on the runtime Java version. > > > > > > Right now I have made some changes in > > > https://github.com/tcurdt/jdependency/pull/209 and > > > https://github.com/apache/maven-shade-plugin/pull/202 > > > > > > Right now the m-shade-p build fails over a few tests that are related > to > > > the META-INF/MANIFEST.MF file. > > > > > > I have trouble figuring out what is going wrong. > > > > > > *I would really appreciate it if someone can have a peek at my efforts > > and > > > provide feedback.* > > > > > > My key questions: > > > > > > - Is this a correct implementation direction? All the m-shade-p code > > > assumes the path of the class file to be the same (if you replace > '.' > > > and > > > '/') as the packagename+classname. I have tried to keep the impact > > > limited. > > > Perhaps a much more impactful change is needed where for all files a > > > record > > > is introduced which contains a set of properties (like classname and > > > filename separately). > > > - What did I do to break the META-INF/MANIFEST.MF related tests? I > did > > > add the extra flag to indicate it is a multi release jar because > that > > is > > > needed by the java runtime, but other than that I've tried to not > > touch > > > it > > > (and apparently failed). > > > > > > Thanks for the feedback > > > > > > -- > > > Best regards / Met vriendelijke groeten, > > > > > > Niels Basjes > > > > > > > > -- > Best regards / Met vriendelijke groeten, > > Niels Basjes >
