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

The fileset classes not present in resulting index #138

Closed
mvysny opened this issue Jan 21, 2021 · 11 comments
Closed

The fileset classes not present in resulting index #138

mvysny opened this issue Jan 21, 2021 · 11 comments
Assignees
Labels
breaking-change Changes that break API backwards compatibility maven-plugin
Milestone

Comments

@mvysny
Copy link

mvysny commented Jan 21, 2021

I have the following maven project: pom.xml is as follows:

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>

    <groupId>com.github.mvysny.vaadin-jandex</groupId>
    <artifactId>vaadin-jandex</artifactId>
    <version>14.4.6-SNAPSHOT</version>

    <properties>
        <maven.compiler.source>8</maven.compiler.source>
        <maven.compiler.target>8</maven.compiler.target>
        <vaadin.version>14.4.6</vaadin.version>
    </properties>
    <dependencyManagement>
        <dependencies>
            <dependency>
                <groupId>com.vaadin</groupId>
                <artifactId>vaadin-bom</artifactId>
                <version>${vaadin.version}</version>
                <type>pom</type>
                <scope>import</scope>
            </dependency>
        </dependencies>
    </dependencyManagement>
    <dependencies>
        <dependency>
            <groupId>com.vaadin</groupId>
            <artifactId>vaadin</artifactId>
            <optional>true</optional>
            <exclusions>
                <!-- Webjars are only needed when running in Vaadin 13 compatibility mode -->
                <exclusion>
                    <groupId>com.vaadin.webjar</groupId>
                    <artifactId>*</artifactId>
                </exclusion>
                <exclusion>
                    <groupId>org.webjars.bowergithub.insites</groupId>
                    <artifactId>*</artifactId>
                </exclusion>
                <exclusion>
                    <groupId>org.webjars.bowergithub.polymer</groupId>
                    <artifactId>*</artifactId>
                </exclusion>
                <exclusion>
                    <groupId>org.webjars.bowergithub.polymerelements</groupId>
                    <artifactId>*</artifactId>
                </exclusion>
                <exclusion>
                    <groupId>org.webjars.bowergithub.vaadin</groupId>
                    <artifactId>*</artifactId>
                </exclusion>
                <exclusion>
                    <groupId>org.webjars.bowergithub.webcomponents</groupId>
                    <artifactId>*</artifactId>
                </exclusion>
            </exclusions>
        </dependency>
    </dependencies>
    <build>
        <plugins>
            <!-- in order to build Jandex index for Vaadin jars, we need to unpack them first -->
            <plugin>
                <artifactId>maven-dependency-plugin</artifactId>
                <version>3.1.2</version>
                <executions>
                    <execution>
                        <id>unpack-dependencies</id>
                        <phase>process-sources</phase>
                        <goals>
                            <goal>unpack-dependencies</goal>
                        </goals>
                    </execution>
                </executions>
                <configuration>
                    <includeGroupIds>com.vaadin</includeGroupIds>
<!--                    <outputDirectory>${project.build.directory}/classes</outputDirectory>-->
                </configuration>
            </plugin>
            <plugin>
                <groupId>org.jboss.jandex</groupId>
                <artifactId>jandex-maven-plugin</artifactId>
                <version>1.0.8</version>
                <executions>
                    <execution>
                        <id>make-index</id>
                        <goals>
                            <goal>jandex</goal>
                        </goals>
                        <!-- phase is 'process-classes by default' -->
                    </execution>
                </executions>
                <configuration>
                    <fileSets>
                        <fileSet>
                            <directory>${project.build.directory}/dependency</directory>
                        </fileSet>
                    </fileSets>
                </configuration>
            </plugin>
        </plugins>
    </build>
</project>

Running mvn clean package on this project will result in an empty Jandex index being produced (the jandex.idx only has 18 bytes).

Running with mvn clean package -X shows that Jandex plugin actually walks over the Vaadin classes, it simply won't include those in the resulting index:

...
[INFO] Indexed com.vaadin.flow.internal.BrowserLiveReload$Backend (0 annotations)
[INFO] Indexed com.vaadin.flow.internal.UsageStatistics$1 (0 annotations)
[INFO] Indexed com.vaadin.flow.internal.StateNode$1 (0 annotations)
[INFO] Indexed com.vaadin.flow.internal.UsageStatistics (0 annotations)
[INFO] Indexed com.vaadin.flow.internal.UsageStatistics$UsageEntry (0 annotations)
[INFO] Indexed com.vaadin.flow.internal.StateNode$FeatureSetKey (0 annotations)
[INFO] Indexed com.vaadin.flow.internal.ResponseWriter (1 annotations)
...
@mvysny
Copy link
Author

mvysny commented Jan 21, 2021

However, uncommenting the <outputDirectory>${project.build.directory}/classes</outputDirectory> will cause the dependency plugin to unpack all classes into target/classes/ causing the Jandex plugin to index the classes correctly: the jandex.idx now has ~600kb. The problem is that all classes are now packaged into the resulting jar file.

@famod
Copy link
Contributor

famod commented Jan 23, 2021

I had a quick look at this.
The problem is that the index file is created in the fileset directory target/dependency/META-INF/jandex.idx and not in target/classes/META-INF/jandex.idx where the jar-plugin would pick it up.

I'm not sure how this is supposed to work, but I would have expected all fileset indexes to be merged into one big index file...? @n1hility @stuartwdouglas

Anyway, @mvysny a workaround for you would be to configure jar-plugin to either include target/dependency/META-INF/jandex.idx or to exclude all (Vaadin) classes.

@mvysny
Copy link
Author

mvysny commented Jan 25, 2021

Excellent catch, I have completely missed the target/dependency/META-INF/jandex.idx file. I would also expect just one big index file to be produced.

I went with the other workaround to exclude all classes (or rather only to include the jandex.idx file):

            <plugin>
                <!-- workaround for https://github.com/wildfly/jandex-maven-plugin/issues/25 -->
                <artifactId>maven-jar-plugin</artifactId>
                <version>3.2.0</version>
                <configuration>
                    <includes>
                        <include>**/jandex.idx</include>
                    </includes>
                </configuration>
            </plugin>

@Ladicek
Copy link
Contributor

Ladicek commented Aug 31, 2021

I just wanted to add a test for the fileSets feature (because I rewrote the Mojo to use actual Java annotations instead of javadoc annotations) and found this issue. So yea, currently, an extra index is created for each fileSet, which I'm not sure is intentional or not. I'd personally also expect a single index for all fileSets.

@Ladicek
Copy link
Contributor

Ladicek commented Aug 31, 2021

I'm working on moving the Jandex Maven plugin into the Jandex codebase and I guess we can change this, as part of Jandex 3.0 release, and document as breaking change :-)

@famod
Copy link
Contributor

famod commented Aug 31, 2021

I'd personally also expect a single index for all fileSets

I changed my mind on that: In my current project we (for some time) had an index just for the test branch of a module that was included into the respective test-jar. We switched to beans.xml because we now produce two test-jars (with different content) and so one test index doesn't cut it.
But now, due to quarkusio/quarkus#19030 (comment), we'd actually need to create two distinct test index files (and an adjustment in Quarkus) to fully achieve what we "need".
So I think we need more flexibility: combined index file, n separate index files etc.

@Ladicek
Copy link
Contributor

Ladicek commented Aug 31, 2021

Whoah that's an interesting one. Here's my take on it:

  • single execution of the jandex goal should IMHO produce a single index
  • if you need to produce multiple indices (for some reason, I don't like the word "indexes" 🤣), you should have multiple executions of the jandex goal

WDYT?

@Ladicek
Copy link
Contributor

Ladicek commented Aug 31, 2021

Some more config would be required for that actually (being able to configure not just the file name, but also full file path), but that should be doable.

@famod
Copy link
Contributor

famod commented Aug 31, 2021

Well, right after I submitted my previous comment, it occured to me that I had not considered mutliple executions! So I think that's a 👍 😉

I've also realized that it won't be that easy to include different index files in different test-jars with standard maven, but that's not the problem of this plugin.

for some reason, I don't like the word "indexes"

Haha, I tried to avoid it as well in my last comment, but I failed in my first one.

@tjquinno
Copy link

FWIW, another 👍 for one execution -> one index.

I also opened a separate but somewhat related enhancement issue wildfly/jandex-maven-plugin#33 in case you feel like to have extra spare time!

@Ladicek Ladicek transferred this issue from wildfly/jandex-maven-plugin Sep 2, 2021
@Ladicek Ladicek self-assigned this May 10, 2022
@Ladicek Ladicek added this to the 3.0.0 milestone May 10, 2022
@Ladicek Ladicek added the breaking-change Changes that break API backwards compatibility label May 10, 2022
@Ladicek
Copy link
Contributor

Ladicek commented May 11, 2022

Done in #195.

@Ladicek Ladicek closed this as completed May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Changes that break API backwards compatibility maven-plugin
Projects
None yet
Development

No branches or pull requests

4 participants