Skip to content
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

Weaver doesn't consider transitive dependencies #69

Open
marcopelegrini opened this issue Jan 30, 2021 · 9 comments
Open

Weaver doesn't consider transitive dependencies #69

marcopelegrini opened this issue Jan 30, 2021 · 9 comments

Comments

@marcopelegrini
Copy link

If you include a dependency to be weaved that has a transitive dependency that you also wants to weave, this later is not considered unless explicitly specified.
Is there a workaround for that?

@marcopelegrini marcopelegrini changed the title Waver doesn't consider transitive dependencies Weaver doesn't consider transitive dependencies Jan 30, 2021
@marcopelegrini
Copy link
Author

Here's a workaround I've created... if there's an easier way, please let me know:

<plugins>
                    <plugin>
                        <groupId>org.apache.maven.plugins</groupId>
                        <artifactId>maven-dependency-plugin</artifactId>
                        <configuration>
                            <includeGroupIds>${project.groupId}</includeGroupIds>
                            <outputScope>false</outputScope>
                            <outputAbsoluteArtifactFilename>true</outputAbsoluteArtifactFilename>
                            <includeTypes>jar</includeTypes>
                            <outputFile>dependencies.txt</outputFile>
                        </configuration>
                        <executions>
                            <execution>
                                <phase>compile</phase>
                                <goals>
                                    <goal>list</goal>
                                </goals>
                            </execution>
                        </executions>
                    </plugin>
                    <plugin>
                        <groupId>org.codehaus.gmaven</groupId>
                        <artifactId>groovy-maven-plugin</artifactId>
                        <version>2.1.1</version>
                        <executions>
                            <execution>
                                <phase>compile</phase>
                                <goals>
                                    <goal>execute</goal>
                                </goals>
                                <configuration>
                                    <source>
                                        project.properties.projectDependencies =
                                                new File("dependencies.txt")
                                                .text
                                                .readLines()
                                                .findAll { it.contains(project.groupId) }
                                                .collect { it.split(":", 5)[4] }
                                                .join(':')
                                    </source>
                                </configuration>
                            </execution>
                        </executions>
                    </plugin>
                    <plugin>
                        <groupId>org.codehaus.mojo</groupId>
                        <artifactId>aspectj-maven-plugin</artifactId>
                        <version>1.11</version>
                        <configuration>
                            <complianceLevel>1.8</complianceLevel>
                            <source>1.8</source>
                            <target>1.8</target>
                            <weaveDirectories>
                                <weaveDirectory>${projectDependencies}</weaveDirectory>
                            </weaveDirectories>
                        </configuration>
                        <executions>
                            <execution>
                                <goals>
                                    <goal>compile</goal>
                                </goals>
                            </execution>
                        </executions>
                    </plugin>
                </plugins>

@bmarwell
Copy link
Contributor

If you include a dependency to be weaved that has a transitive dependency that you also wants to weave,

Hi @marcopelegrini,
can you please post the original configuration which did not work?
I guess you are using this setup?

<configuration>
      <weaveDependencies>
        <weaveDependency>  
            <groupId>org.agroup</groupId>
            <artifactId>to-weave</artifactId>
        </weaveDependency>
        <weaveDependency>
            <groupId>org.anothergroup</groupId>
            <artifactId>gen</artifactId>
        </weaveDependency>
    </weaveDependencies>
</configuration>

If so, no, transitive dependencies are not supported yet. PRs are very welcome, though. :)

@marcopelegrini
Copy link
Author

Hi @bmarwell that's exactly right... so looks like it's not an issue but a feature request :)
I'll definitely try to find some time to provide a solution, thanks for checking on this.

@bmarwell
Copy link
Contributor

Sounds good to me! I'd make it not default, because I think the dependent transitive jars should already contain the woven classes, or am I missing something?

@kriegaex
Copy link
Collaborator

kriegaex commented Jun 1, 2021

I would be very careful to implement this. If at all, it should be opt-in, like Ben said, because it would break backward compatibility. It is just not how Ajc works. You usually do not want to weave the world, but be explicit about what you want woven. Granted, a Maven plugin is an abstraction layer and not necessarily a 1-to-1 mapping to underlying tool parameters, so I can imagine this as a convenience feature. But spontaneously, I am skeptical and would rather not implement it. But that is, of course, up to the team of plugin maintainers.

We should stop for a moment and think about how other plugins handle transitive dependencies and what would be the most probable user expectation. Please also consider the test matrix that would ensue for such a feature. Last but not least, think about what it would mean for aspect weaving: It would become dependent on how some upstream maintainer of a third-party component changes her POM. The results could be quite unpredictable. OK, opt-in means "use at your own risk", but AspectJ Maven already being "copy & paste programming" in large parts, based on my StackOverflow experience, I am expecting lots of newbies to use it because it sounds cool and promises to save typing and doing POM maintenance work. The result would be lots of support questions.

@bmarwell
Copy link
Contributor

bmarwell commented Jun 1, 2021

Alex, thanks for your insights!
I was close to tag this as "wont-fix", but it seems I got the right gut feeling. You can use the dependency plugin and unpack your dependencies first, ofcourse.

I am skeptical and would rather not implement it. But that is, of course, up to the team of plugin maintainers.

I am considering and valuing your input as well!

The results could be quite unpredictable. […] The result would be lots of support questions.

That is what I fear most. Even with opt-in, I think builds would break randomly because some of your project's dependencies introduced some breaking change.

@marcopelegrini can you please give us a use case?

@marcopelegrini
Copy link
Author

Yes, for sure... if you look at my workaround you kinda grasp what I'm trying to do..

I have a multi-module project, layered out like this:

[INFO] ------------------< my.groupid:depA >------------------
[INFO] my.groupid:depA
[INFO] ------------------< my.groupid:depB >------------------
[INFO] my.groupid:depB
[INFO] +- my.groupid:depA
[INFO] ------------------< my.groupid:depC >------------------
[INFO] my.groupid:depC
[INFO] +- my.groupid:depB
[INFO] |  +- my.groupid:depA
[INFO] ----------------< my.groupid:app >----------------
[INFO] my.groupid:app
[INFO] +- my.groupid:depC
[INFO] |  +- my.groupid:depB
[INFO] |  |  +- my.groupid:depA

So if I configure the plugin in the app project, and add depC depB depA as weave dependencies, only depC is weaved. I want all three to be.

In my case the ideal configuration supported would be some sort of pattern based configuration, because I prefer not to specify all transitive artifactIds (they might change over time), but I want to weave every dependency that's part of my own groupId.

<configuration>
    <weaveDependencies>
        <weaveDependency>
            <groupId>my.groupId</groupId>
            <artifactId>*</artifactId>
        </weaveDependency>
    </weaveDependencies>
</configuration>

@kriegaex
Copy link
Collaborator

kriegaex commented Jun 2, 2021

        <weaveDependency>
            <groupId>my.groupId</groupId>
            <artifactId>*</artifactId>

That is something completely different and much simpler than transitive dependency inclusion. You simply want to use patterns for artifact IDs or maybe also groupIDs. That would rather help for the case of multiple direct dependencies from the same group ID. I would not expect that this also applies recursively. Recursion into the dependency tree via something like

<configuration>
    <includeTransitiveWeaveDependencies>true</includeTransitiveWeaveDependencies>
    <weaveDependencies>
        <weaveDependency>
            <groupId>my.groupId</groupId>
            <artifactId>A</artifactId>
        </weaveDependency>
    </weaveDependencies>
</configuration>

or, more specific,

<configuration>
    <weaveDependencies>
        <weaveDependency>
            <groupId>my.groupId</groupId>
            <artifactId>A</artifactId>
            <includeTransitive>true</includeTransitive>
        </weaveDependency>
    </weaveDependencies>
</configuration>

really would be a separate feature, which - if active - ideally would be usable in combination with name filtering. Something like

<configuration>
    <weaveDependencies>
        <weaveDependency>
            <groupId>my.groupId</groupId>
            <artifactId>A</artifactId>
           <transitive>
             <includes>my.groupId:*,org.apache.*:*</includes>
             <excludes>org.apache.*:ant*</excludes>
           <transitive>
        </weaveDependency>
    </weaveDependencies>
</configuration>

Because it would be the next logical thing users would come up with: to demand that the feature supports situations like "I want to include transitive dependencies, but not all of them". But then we are starting to rebuild logic like in Maven Shade.

@marcopelegrini
Copy link
Author

That is something completely different and much simpler than transitive dependency inclusion.

For sure, that's a different solution.
I originally mentioned transitive because (on my example) if you have a transitive dependency. e.g. depA and you add it to the dependencies to weave, the plugin won't consider it, only the ones explicitly defined in the project e.g. depC, but then it would defeat the purpose of transitive dependencies.

You simply want to use patterns for artifact IDs or maybe also groupIDs.

Yes, so this would ignore where the dependency is coming from, and rather weave them based on that pattern:

<configuration>
    <weaveDependencies>
        <includes>my.groupId:*,org.apache.*:*</includes>
        <excludes>org.apache.*:ant*</excludes>
    </weaveDependencies>
</configuration>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants