Mng 6957: Support Build Consumer while defining modules in reverse order

[rfscholte]

Original summary: cannot build Maven Archetype with versionless reactor atifacts

[gnodet]

I think the problem raised in 331e402 and aa285f2 are still valid with the proposed PR. Actually a bit worse since the ReactorModelCache now contains not only model data, but also a mapping from GA coordinates to Source. Maybe this map could be set on the DefaultTransformerContext. In all cases, the GA<->Source mapping is not really used as a cache (a cache should be optional and just avoids computing things multiple times) but as a storage (the findRawModel returns null the mapping has not been stored in the cache previously).

I'm willing to rework my work on top of yours, but the above point makes it much more difficult. Could you consider revisit this point somehow ?

[gnodet]

I think the problem raised in 331e402 and aa285f2 are still valid with the proposed PR. Actually a bit worse since the ReactorModelCache now contains not only model data, but also a mapping from GA coordinates to Source. Maybe this map could be set on the DefaultTransformerContext. In all cases, the GA<->Source mapping is not really used as a cache (a cache should be optional and just avoids computing things multiple times) but as a storage (the findRawModel returns null the mapping has not been stored in the cache previously).

I'm willing to rework my work on top of yours, but the above point makes it much more difficult. Could you consider revisit this point somehow ?

@rfscholte I went ahead and did a small refactoring to avoid using the reactor cache, see gnodet@497e7fe

[michael-o]

I'd like to mention MNG-6983 which causes out of sync issues.

[michael-o]

I guess there is no way to break this up in a few logical PRs?

[rfscholte]

All is required to fix the issue, there's no clear way to split it.

[michael-o]

OK, thanks.

[gnodet]

@rfscholte overall, because of the number of commits, it's really difficult to comment on the diffs, maybe at this point, a squashed commit would be easier to review, since there are so many commits... Hence I'm raising a few points here rather than in possible diff comments...

File vs Raw vs Effective model

The concept behind the raw model seems to change quite a bit. I've watched the video, so I roughly get the idea. However, there are a few spots where things are unclear : for example the buildRawModel() method from the DefaultModelBuilder seems to return... a file model. It would be nice to have it documented somewhere.

FileModelSource vs ArtifactModelSource

Could you please clarify the usage of FileModelSource ? There are a few spots where modelSource instanceof FileModelSource is used. It seems that FileModelSource is used to denote a pom file which is being built (i.e. part of the reactor) whereas ArtifactModelSource denotes a pom file coming from a repository. What is unclear is why would is why the process would be specific to the Source implementation... If pom files should be read differently, shouldn't that be an explicit part of the BuildModelRequest instead of implicit from the type of the Source ?

GA instead of GAV

All the model read seem to be indexed by group/artifact and not by group/artifact/version. Does that mean that a single reactor can not used 2 different dependencies with different versions ? I know this can't happen for a single model, but this is currently supported for two different modules. If the reactor only contains a single model for a given GA, how will that work ?

SAX vs Stax

Another point is the usage of SAX rather than Stax API (honestly, I have hardly seen anybody using SAX since years). The models are read using a pull parser, so I'm not sure why introducing a push SAX filter to convert the file into a stream to be consumed by the pull parser in another thread. This seems really inefficient. In huge builds, a few hundreds or more pom files can be read, so that could really be a real problem. I wonder if the ModelSourceTransformer would be better redesigned as a pull filter somehow. Maybe something like

   XmlPullParser transform( XmlPullParser parser, TransformerContext context )
       throws IOException, TransformerException;

I know there are a bunch of things written in maven-xml already but this decision can have a big impact. An alternative would be to only use a SAX parser, in which case the xml -> model mapping would have to be rewritten. I don't really care, but it does not seem a very good option for the future mix push/pull parsing together to read a model.

ModelReader

In the DefaultModelReader, only the Model read( File input, Map<String, ?> options ) method does use the ModelSourceTransformer to read the Model. If there's a good reason for not performing a transformation when using a Reader or an InputStream, it should be written in the javadoc, but it seems to me it can be extended to the other methods as well.

[rfscholte]

@gnodet thanks for reviewing. It doesn't make sense to review it per commit, I would suggest to comment on the total changes. In the end these commit will be squashed into on, yes.

File vs Raw vs Effective model
ModelBuilder.buildRawModel( File pomFile, int validationLevel, boolean locationTracking ) was actually never used in our codebase. Now returning the rawModel only works if there is a fileModel. This is different compared to Maven3, where the fileModel and rawModel are actually the same.

FileModelSource vs ArtifactModelSource
Yes, artifactModelSources are loaded from the repository, FileModelSources are from the reactor/multimodule. The main reason is that pom from a repository should NEVER be transformed.

GA instead of GAV
GA is required for versionless dependencies, i.e. if there is only one GA in the project, now we can use that version. If for a weird reason there are multiple versions, an exception is thrown, because Maven cannot select a version: https://github.com/apache/maven/pull/391/files#diff-8af144c3b53554dd804a30126d20926a7f93aa03b86fdc12613a2f5c2df2b15cR57-R72

SAX vs Stax
I don't see the issues here. There is a need for chaining, hence the available XMLFilters made sense to me. For now I would like to stick to the current implementation, do an alpha-release and wait for feedback.

ModelReader
IIRC when I tried that several tests broke and I couldn't figure out how to solve that.

[gnodet]

@rfscholte I've pushed a better commit for the ReactorModelCache at gnodet@e60fce4
There's stil the cast in DefaultModelBuilder (which could be transformed into an assertion given the TransformerContextBuilder is created by the DefaultModelBuilder, but the TranformerContext which is supposed to be immutable does not have the putSource method anymore.

[rfscholte]

@gnodet I've added commit, see 50e8beb

[michael-o]

@rfscholte Please also rebase/merge master into this branch. I'd like to run all ITs with that.

[michael-o]

One more nit, but I cannot object anything here anymore. Going through ITs now.

[michael-o]

LGTM now. Please squash along with ITs, I will run again.