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

microsoft-kiota-** dependencies have conflicting Automatic-Module-Name #1108

Open
dnijssen opened this issue Mar 2, 2024 · 5 comments
Open
Labels
bug Something isn't working priority:p3 Nice to have. Customer impact is very minimal type:breaking-change An issue that will result in dependent client projects failing.
Milestone

Comments

@dnijssen
Copy link

dnijssen commented Mar 2, 2024

We use for our projects both jdeps and jlink tools to generate a slim JRE. However since the switch to com.microsoft.graph:microsoft-graph:6.4.0 (v6 of the SDK, recently released), which builds on top of the microsoft-kiota-** dependencies. We can't seem to run our jdeps command succesfully anymore.

Previously (without the microsoft-kiota-** dependencies on the class-/module path we have been using the following command; (exectued from target/)

jdeps --multi-release base --recursive --ignore-missing-deps --print-module-deps --class-path="./lib/*" --module-path="./lib/*" -quiet demo-0.0.1-SNAPSHOT.jar

Which would list something like this; java.base,spring.boot,spring.boot.autoconfigure,spring.context,spring.core

Note: When running this on a Windows machine, be sure to change ./lib/* to ./lib (as there is an issue with the * wildcard.

Howafter after switching to com.microsoft.graph:microsoft-graph:6.4.0 we not get the following exception when we run the same command;

Exception in thread "main" java.lang.module.FindException: Two versions of module com.microsoft.kiota found in .\lib (microsoft-kiota-authentication-azure-1.0.4.jar and microsoft-kiota-abstractions-1.0.4.jar)
        at java.base/jdk.internal.module.ModulePath.scanDirectory(ModulePath.java:295)
        at java.base/jdk.internal.module.ModulePath.scan(ModulePath.java:233)
        at java.base/jdk.internal.module.ModulePath.scanNextEntry(ModulePath.java:191)
        at java.base/jdk.internal.module.ModulePath.findAll(ModulePath.java:167)
        at jdk.jdeps/com.sun.tools.jdeps.JdepsConfiguration$Builder.build(JdepsConfiguration.java:558)
        at jdk.jdeps/com.sun.tools.jdeps.JdepsTask.buildConfig(JdepsTask.java:607)
        at jdk.jdeps/com.sun.tools.jdeps.JdepsTask.run(JdepsTask.java:561)
        at jdk.jdeps/com.sun.tools.jdeps.JdepsTask.run(JdepsTask.java:537)
        at jdk.jdeps/com.sun.tools.jdeps.Main.main(Main.java:50)

A quick look here in the repository shows me that all components/**/build.gradle files are containing this piece of code;

tasks.jar {
    manifest {
        attributes('Automatic-Module-Name': project.property('mavenGroupId'))
    }
}

So the Automatic-Module-Name is set to an unique value, which isn't correct in my opinion. Might be better to use the artifactId here maybe? But that is up to you guys ;)

Following pom.xml file can be used to simulate this issue. Note the maven-dependency-plugin which copies all the dependencies (on mvn install) to target/lib for easier testing with the command.

<?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 https://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>
    <parent>
        <groupId>org.springframework.boot</groupId>
        <artifactId>spring-boot-starter-parent</artifactId>
        <version>3.2.3</version>
        <relativePath/> <!-- lookup parent from repository -->
    </parent>
    <groupId>com.example</groupId>
    <artifactId>demo</artifactId>
    <version>0.0.1-SNAPSHOT</version>
    <name>demo</name>
    <description>Demo project for Spring Boot</description>
    <properties>
        <java.version>21</java.version>
        <microsoft-graph.version>6.4.0</microsoft-graph.version>
        <azure-identity.version>1.11.2</azure-identity.version>
    </properties>
    <dependencies>
        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter</artifactId>
        </dependency>
        <dependency>
            <groupId>com.microsoft.graph</groupId>
            <artifactId>microsoft-graph</artifactId>
            <version>${microsoft-graph.version}</version>
        </dependency>
        <dependency>
            <groupId>com.azure</groupId>
            <artifactId>azure-identity</artifactId>
            <version>${azure-identity.version}</version>
            <scope>compile</scope>
        </dependency>
    </dependencies>

    <build>
        <plugins>
            <plugin>
                <groupId>org.springframework.boot</groupId>
                <artifactId>spring-boot-maven-plugin</artifactId>
            </plugin>
            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-dependency-plugin</artifactId>
                <executions>
                    <execution>
                        <phase>install</phase>
                        <goals>
                            <goal>copy-dependencies</goal>
                        </goals>
                        <configuration>
                            <outputDirectory>${project.build.directory}/lib</outputDirectory>
                        </configuration>
                    </execution>
                </executions>
            </plugin>
        </plugins>
    </build>

</project>
@github-project-automation github-project-automation bot moved this to Todo in Kiota Mar 2, 2024
@baywet baywet added bug Something isn't working Standard GitHub label Issue caused by core project dependency modules or library labels Mar 4, 2024
@baywet baywet added this to the Kiota v1.13 milestone Mar 4, 2024
@baywet
Copy link
Member

baywet commented Mar 4, 2024

Hi @dnijssen
Thanks for using the Microsoft Graph SDK and for bringing this up.
This is most likely because we've copy/pasted some of the build.gradle sections from microsoft graph 5.X without thoroughly evaluating the impacts at the time.
After reading this guidance, a couple of things seem to stand out (please correct me if I'm wrong in my evaluation):

  • references are only broken for people using the packages in a "automatic module mode" instead of the default jar mode.
  • changing the module name WILL NOT impact the people using the default jar resolution.
  • module names need to be unique.
  • setting different module names WILL NOT make a difference to people using automatic module mode (import paths, etc...) when compared with the default jar mode.

If all that holds true, I'd like to suggest the following module names:

  • abstractions: com.microsoft.kiota.abstractions
  • json serialization: com.microsoft.kiota.serialization.json
  • text serialization: com.microsoft.kiota.serialization.text
  • form serialization: com.microsoft.kiota.serialization.form
  • multipart serialization: com.microsoft.kiota.serialization.multipart
  • azure authentication: com.microsoft.kiota.authentication.azure
  • okhttp http: com.microsoft.kiota.http.okhttp

Thoughts?

I'd also like to get @andreaTP opinion on the matter.

@andreaTP
Copy link
Contributor

andreaTP commented Mar 4, 2024

Thanks for looping me in @baywet , in my experience, it's rare that ppl will use jlink but it is a valid use-case and we want to support it.
I don't know what would be the most "correct" resolution though.

@andrueastman andrueastman modified the milestones: Kiota v1.13, Kiota v1.14 Apr 8, 2024
@dnijssen
Copy link
Author

dnijssen commented Apr 8, 2024

I see this issue has been moved to the v1.14 milestone, is there anything holding this issue up? Would love to see this issue resolved, suggested module names sounds more than right in my opinion :)

@baywet
Copy link
Member

baywet commented Apr 15, 2024

@dnijssen this has been moved from one milestone to another as part of our release process (1.13 was released) but nobody is working on that at the moment (staff shortage/other priorities).
We'll be more than happy to review a PR if you are willing to submit one, as far as I understand, the only required work here is to update the header

jar {
    manifest {
        name = "mylibrary"
        instruction "Automatic-Module-Name", "com.acme.mylibrary"
    }
}

attributes('Automatic-Module-Name': project.property('mavenGroupId'))

@baywet
Copy link
Member

baywet commented Apr 18, 2024

with the discussion in #1224 we realized that we have split packages, which would require moving classes to different packages to fully implement that change. We're unlikely to so that soon since it'd represent a source breaking change. Queuing for the next major version.

@baywet baywet added type:breaking-change An issue that will result in dependent client projects failing. priority:p3 Nice to have. Customer impact is very minimal and removed Standard GitHub label Issue caused by core project dependency modules or library labels Apr 18, 2024
@baywet baywet modified the milestones: Kiota v1.14, Backlog Apr 18, 2024
@baywet baywet moved this from Todo 📃 to Blocked 🔒 in Kiota Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p3 Nice to have. Customer impact is very minimal type:breaking-change An issue that will result in dependent client projects failing.
Projects
Status: Blocked 🔒
Development

No branches or pull requests

4 participants