repaint-io / maven-tiles

Injecting maven configurations by composition rather than inheritance
154 stars 32 forks source link

Filtering not applied when tile is part of build #83

Closed bvella closed 5 years ago

bvella commented 5 years ago

When running a build in which the a tile is used and in the list of projects to be built, and the tile has some properties to be replaced, these are not replaced.

I has a look at the code and this is due to the fact that in TilesMavenLifecycleParticipant.resolveTile() there a piece of code that goes through the list of projects in the maven session and if one matches a tile, the tile.xml file is used directly rather than resolving the tile from the repo. This is fine for tiles that do not replace property values using filtering but breaks tiles that do.

I have a project that has this situation. what I used to do before (with version 2.10) is first build the tile then build everything (including the tiles). However, with version 2.12, this breaks and I would have to separate my tiles from the rest of the code. I know I can solve this by breaking up the project into smaller projects, which will be done in due course, but then this pattern would not be needed anyway.

I tried (and managed) to add filtering during the lifecycle, however the code turned out quite ugly and requires a number of assumptions. I would suggest to

rvowles commented 5 years ago

The tile is used directly because otherwise tiles in a reactor cannot be used. So it makes sense to use your second option, and you are welcome to submit code!

On Sun, Dec 23, 2018 at 11:23 PM Brian Vella notifications@github.com wrote:

When running a build in which the a tile is used and in the list of projects to be built, and the tile has some properties to be replaced, these are not replaced.

I has a look at the code and this is due to the fact that in TilesMavenLifecycleParticipant.resolveTile() there a piece of code that goes through the list of projects in the maven session and if one matches a tile, the tile.xml file is used directly rather than resolving the tile from the repo. This is fine for tiles that do not replace property values using filtering but breaks tiles that do.

I have a project that has this situation. what I used to do before (with version 2.10) is first build the tile then build everything (including the tiles). However, with version 2.12, this breaks and I would have to separate my tiles from the rest of the code. I know I can solve this by breaking up the project into smaller projects, which will be done in due course, but then this pattern would not be needed anyway.

I tried (and managed) to add filtering during the lifecycle, however the code turned out quite ugly and requires a number of assumptions. I would suggest to

  • either remove this part of the lifecycle, and let tiles only be resolved from the repo or
  • do the filtering during the lifecycle as well (I have code for this which I can submit for opinions. I would rather not submit it as a pull request yet until we get an agreement on the assumptions)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/repaint-io/maven-tiles/issues/83, or mute the thread https://github.com/notifications/unsubscribe-auth/AADZ2KnistGnzjb-RX9i43A4aHoSlMy2ks5u71mhgaJpZM4ZfyVh .

--

Richard Vowles, Full stack - from Kubernetes, through Node & Java, Web and Mobile development in Flutter - software developer for hire!

ph: +64275467747, web: www.google.com/+RichardVowles

bvella commented 5 years ago

Ok, so this is what I did:

I extracted the getTile() method fromAbstractTileMojo to a common helper class to be able to call the same code from TilesMavenLifecycleParticipant:

tileArtifact.file = new File(currentProject.file.parent, "tile.xml")

becomes

tileArtifact.file = FilteringHelper.getTile(currentProject, new File(currentProject.build.directory, "tmp-tile"), mavenSession, logger)

The problems I encountered are twofold, maybe because I am not that familiar with maven plugins and lifecycles:

  1. I am determining whether to do filtering by looking through the project's plugins to match tiles-maven-plugin, casting the configuration to Xpp3Dom and looking for the filtering xml property. I don't know if there is a better, more maven friendly, way
  2. I use reflection to set up an instance of MavenFileFilter and MavenResourceFiltering as these are not injectable as components in the lifecycle as they are in the Mojos.

With these changes, my project builds fine. If you have any suggestions I will fix this up and submit as a pull request. If you prefer to discuss this in a pull request, I will gladly create the pull request with the code changes as they are now.

The helper class looks like this:

class FilteringHelper {

    public static final String TILE_POM = "tile.xml"

    static File getTile(final MavenProject project,
                        final File generatedSourcesDirectory,
                        final MavenSession mavenSession,
                        final Logger logger ) {
        // determine whether filtering is enabled
        final List<Plugin> plugins = project.build.plugins
            .stream()
            .filter { p -> p.groupId == "io.repaint.maven" && p.artifactId == "tiles-maven-plugin" }
            .collect()

        final boolean filtering = (plugins.size() == 1) && ((plugins[0].configuration as Xpp3Dom)?.getChild("filtering")?.getValue() == "true")

        if (filtering) {
            // Big string of assumptions here: 
            // getTile() makes use of built-in maven filtering that uses an implementation of MavenFileFilter and MavenResourcesFiltering.
            // These components are injected in the Mojo with @Component, but are unavailable to the maven lifecycle participant.
            // I attempted to supply an implementation myself by looking for available implementations and looking at what implementations
            // maven uses at runtime (using breakpoints and IDE variable inspection). I then brute force set all private fields that caused
            // NullPointerExceptions by reflection with an appropriate implementation.
            final def context = new DefaultBuildContext()

            final def mavenFileFilter = new DefaultMavenFileFilter()
            mavenFileFilter.enableLogging(logger)
            final def buildContextField = DefaultMavenFileFilter.getDeclaredField("buildContext")
            buildContextField.setAccessible(true)
            buildContextField.set(mavenFileFilter, context)
            final def readerFilterField = DefaultMavenFileFilter.getDeclaredField("readerFilter")
            readerFilterField.setAccessible(true)
            readerFilterField.set(mavenFileFilter, new DefaultMavenReaderFilter())

            final def mavenResourcesFiltering = new DefaultMavenResourcesFiltering()
            mavenResourcesFiltering.initialize()
            mavenResourcesFiltering.enableLogging(logger)
            final def buildContextField2 = DefaultMavenResourcesFiltering.getDeclaredField("buildContext")
            buildContextField2.setAccessible(true)
            buildContextField2.set(mavenResourcesFiltering, context)
            final def mavenFileFilterField = DefaultMavenResourcesFiltering.getDeclaredField("mavenFileFilter")
            mavenFileFilterField.setAccessible(true)
            mavenFileFilterField.set(mavenResourcesFiltering, mavenFileFilter)

            return getTile(project, true, generatedSourcesDirectory, mavenSession, mavenFileFilter, mavenResourcesFiltering)
        } else {
            return getTile(project, false, generatedSourcesDirectory, mavenSession, null, null)
        }
    }

    static File getTile(final MavenProject project,
                        final boolean filtering,
                        final File generatedSourcesDirectory,
                        final MavenSession mavenSession,
                        final MavenFileFilter mavenFileFilter,
                        final MavenResourcesFiltering mavenResourcesFiltering) {
        File baseTile = new File(project.basedir, TILE_POM)
        if (filtering) {
            File processedTileDirectory = new File(generatedSourcesDirectory, "tiles")
            processedTileDirectory.mkdirs()
            File processedTile = new File(processedTileDirectory, TILE_POM)

            Resource tileResource = new Resource()
            tileResource.setDirectory(project.basedir.absolutePath)
            tileResource.includes.add(TILE_POM)
            tileResource.setFiltering(true)

            MavenFileFilterRequest req = new MavenFileFilterRequest(baseTile,
                    processedTile,
                    true,
                    project,
                    [],
                    true,
                    "UTF-8",
                    mavenSession,
                    new Properties())
            req.setDelimiters(["@"] as LinkedHashSet)

            MavenResourcesExecution execution = new MavenResourcesExecution(
                    [tileResource], processedTileDirectory, "UTF-8",
                    mavenFileFilter.getDefaultFilterWrappers(req),
                    project.basedir, mavenResourcesFiltering.defaultNonFilteredFileExtensions)

            mavenResourcesFiltering.filterResources(execution)

            return new File(processedTileDirectory, TILE_POM)
        } else {
            return baseTile
        }
    }

}
talios commented 5 years ago

On 23 Dec 2018, at 23:23, Brian Vella wrote:

I has a look at the code and this is due to the fact that in TilesMavenLifecycleParticipant.resolveTile() there a piece of code that goes through the list of projects in the maven session and if one matches a tile, the tile.xml file is used directly rather than resolving the tile from the repo. This is fine for tiles that do not replace property values using filtering but breaks tiles that do.

Sounds like a regression introduced by the change to allow tiles to exist in a reactor build - something I never do personally.


"The ease with which a change can be implemented has no relevance at all to whether it is the right change for the (Java) Platform for all time." — Mark Reinhold.

Mark Derricutt http://www.theoryinpractice.net http://www.chaliceofblood.net http://plus.google.com/+MarkDerricutt http://twitter.com/talios http://facebook.com/mderricutt

rvowles commented 5 years ago

I actually have Brian's issue where we have docker ids in the tile and if the default property is the project artifact, the substitution doesn't happen in the tile and thus the docker image name ends up trying to ne ${project.artifactId} - so this would solve an issue for me.

Brian can you create a PR for us, I'd like to try this out on our builds.

Ta Richard

On Mon, Dec 24, 2018 at 11:24 AM Mark Derricutt notifications@github.com wrote:

On 23 Dec 2018, at 23:23, Brian Vella wrote:

I has a look at the code and this is due to the fact that in TilesMavenLifecycleParticipant.resolveTile() there a piece of code that goes through the list of projects in the maven session and if one matches a tile, the tile.xml file is used directly rather than resolving the tile from the repo. This is fine for tiles that do not replace property values using filtering but breaks tiles that do.

Sounds like a regression introduced by the change to allow tiles to exist in a reactor build - something I never do personally.


"The ease with which a change can be implemented has no relevance at all to whether it is the right change for the (Java) Platform for all time." — Mark Reinhold.

Mark Derricutt http://www.theoryinpractice.net http://www.chaliceofblood.net http://plus.google.com/+MarkDerricutt http://twitter.com/talios http://facebook.com/mderricutt

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/repaint-io/maven-tiles/issues/83#issuecomment-449666900, or mute the thread https://github.com/notifications/unsubscribe-auth/AADZ2BjmZKEl0BX0iQQ7hzwmdI4Li1cnks5u8AK2gaJpZM4ZfyVh .

--

Richard Vowles, Full stack - from Kubernetes, through Node & Java, Web and Mobile development in Flutter - software developer for hire!

ph: +64275467747, web: www.google.com/+RichardVowles

talios commented 5 years ago

On 24 Dec 2018, at 11:29, Richard Vowles wrote:

Brian can you create a PR for us, I'd like to try this out on our builds.

Surely you mean gerrit-forge review/patch-set :)


"The ease with which a change can be implemented has no relevance at all to whether it is the right change for the (Java) Platform for all time." — Mark Reinhold.

Mark Derricutt http://www.theoryinpractice.net http://www.chaliceofblood.net http://plus.google.com/+MarkDerricutt http://twitter.com/talios http://facebook.com/mderricutt

rvowles commented 5 years ago

Although thats a nice idea, I think it is time to review that policy.

On Mon, Dec 24, 2018 at 11:35 AM Mark Derricutt notifications@github.com wrote:

On 24 Dec 2018, at 11:29, Richard Vowles wrote:

Brian can you create a PR for us, I'd like to try this out on our builds.

Surely you mean gerrit-forge review/patch-set :)


"The ease with which a change can be implemented has no relevance at all to whether it is the right change for the (Java) Platform for all time." — Mark Reinhold.

Mark Derricutt http://www.theoryinpractice.net http://www.chaliceofblood.net http://plus.google.com/+MarkDerricutt http://twitter.com/talios http://facebook.com/mderricutt

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/repaint-io/maven-tiles/issues/83#issuecomment-449667390, or mute the thread https://github.com/notifications/unsubscribe-auth/AADZ2BGJmg4qk-GL72QkRgZhtzZ4umD3ks5u8AUogaJpZM4ZfyVh .

--

Richard Vowles, Full stack - from Kubernetes, through Node & Java, Web and Mobile development in Flutter - software developer for hire!

ph: +64275467747, web: www.google.com/+RichardVowles

bvella commented 5 years ago

added pull request #86