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

Please ship a bill of materials dependency #481

Closed
sixcorners opened this issue Jul 5, 2019 · 5 comments
Closed

Please ship a bill of materials dependency #481

sixcorners opened this issue Jul 5, 2019 · 5 comments

Comments

@sixcorners
Copy link

sixcorners commented Jul 5, 2019

A bill of materials or bom dependency is one that can be used to keep the versions of dependencies in sync. Instead of adding a bunch of dependencies and setting them all to version 3.2.2 or using properties you could instead add a bom with version 3.2.2 and omit the versions on the dependencies managed by the bom. It's a good way to not having to repeat yourself if there are a bunch of versions that all need to be set to the same thing. It also makes sure you are using a set of versions that have all been tested to work together. It would be nice if lwjgl provided a lwjgl-bom dependency I could import.

Here are a few examples of them:
https://github.com/apache/logging-log4j2/blob/log4j-2.12.0/log4j-bom/pom.xml
https://github.com/FasterXML/jackson-bom/blob/jackson-bom-2.9.9/pom.xml
https://github.com/quarkusio/quarkus/blob/0.18.0/bom/pom.xml
https://github.com/spring-projects/spring-data-build/blob/2.1.9.RELEASE/bom/pom.xml
https://github.com/spring-projects/spring-boot/blob/v2.1.6.RELEASE/spring-boot-project/spring-boot-dependencies/pom.xml

Here is how I would use one in maven: https://logging.apache.org/log4j/2.x/maven-artifacts.html#Bill_of_Material
Here is how to do it in gradle: https://docs.gradle.org/current/userguide/managing_transitive_dependencies.html#sec:bom_import

@XenoAmess
Copy link

The reason not to do so I guess is people usually not use all of those packages.

@TheMrMilchmann
Copy link
Contributor

You don not need to use all of the packages with a BOM. Instead, when you include the BOM, you still need to declare all of the dependencies but not their versions. So, what this does is effectively allowing you to skip declaring the LWJGL version {numberOfLWJGLDependencies} times by declaring one additional dependency.

Instead of:

dependencies {
    // define dependencies with versions
    implementation("org.lwjgl", "lwjgl", lwjglVersion)
    implementation("org.lwjgl", "lwjgl-glfw", lwjglVersion)
    implementation("org.lwjgl", "lwjgl-opengl", lwjglVersion)
    implementation("org.lwjgl", "lwjgl-stb", lwjglVersion)
    runtimeOnly("org.lwjgl", "lwjgl", lwjglVersion, classifier = lwjglNatives)
    runtimeOnly("org.lwjgl", "lwjgl-glfw", lwjglVersion, classifier = lwjglNatives)
    runtimeOnly("org.lwjgl", "lwjgl-opengl", classifier = lwjglNatives)
    runtimeOnly("org.lwjgl", "lwjgl-stb", lwjglVersion, classifier = lwjglNatives)
}

You could write:

dependencies {
    // import a BOM
    implementation(platform("{BOM_ARTIFACT_COORDINATES}:lwjglVersion"))

    // define dependencies without versions
    implementation("org.lwjgl", "lwjgl")
    implementation("org.lwjgl", "lwjgl-glfw")
    implementation("org.lwjgl", "lwjgl-opengl")
    implementation("org.lwjgl", "lwjgl-stb")
    runtimeOnly("org.lwjgl", "lwjgl", classifier = lwjglNatives)
    runtimeOnly("org.lwjgl", "lwjgl-glfw", classifier = lwjglNatives)
    runtimeOnly("org.lwjgl", "lwjgl-opengl", classifier = lwjglNatives)
    runtimeOnly("org.lwjgl", "lwjgl-stb", classifier = lwjglNatives)
}

TheMrMilchmann added a commit to TheMrMilchmann/lwjgl3 that referenced this issue Aug 17, 2019
TheMrMilchmann added a commit to TheMrMilchmann/lwjgl3 that referenced this issue Aug 17, 2019
TheMrMilchmann added a commit to TheMrMilchmann/lwjgl3 that referenced this issue Aug 17, 2019
Spasi pushed a commit to TheMrMilchmann/lwjgl3 that referenced this issue Aug 28, 2019
TheMrMilchmann added a commit to TheMrMilchmann/lwjgl3 that referenced this issue Aug 29, 2019
Spasi pushed a commit to TheMrMilchmann/lwjgl3 that referenced this issue Aug 29, 2019
@Spasi Spasi closed this as completed in 07b91ad Aug 29, 2019
@Spasi
Copy link
Member

Spasi commented Aug 10, 2020

@TheMrMilchmann Would it be a bad idea to change the native artifact scope to runtime? Why is it compile now, did we have any trouble with the Java module system?

@TheMrMilchmann
Copy link
Contributor

TheMrMilchmann commented Aug 10, 2020

I do not recall any particular issue leading to this decision (though there were some inconsistencies between Maven and Gradle that we have to watch out for when changing this). However, after some testing, it seems like the current approach is the way to go. Specifying <scope>runtime</scope> in the BOM seems to be implicitly overwritten when omitting the scope in the consuming POM.

When using

<dependency>
    <groupId>org.lwjgl</groupId>
    <artifactId>lwjgl</artifactId>
    <classifier>natives-${platform}</classifier>
</dependency>

+- org.lwjgl:lwjgl:jar:natives-windows:3.2.4-SNAPSHOT:compile is printed by mvn dependency:tree. However, using an unmodified BOM and overwriting the scope (by declaring <scope>runtime</scope> in the consuming POM) reports +- org.lwjgl:lwjgl:jar:natives-windows:3.2.4-SNAPSHOT:runtime as expected.
Thus, I'm not even sure now if the scope defined in the BOM contributes to dependency resolution and we should probably remove it. (The JBoss BOM does not define any scopes.)

May I ask why you would want to change it? If there is a particular issue you are facing, I'll have another look at it.

@Spasi
Copy link
Member

Spasi commented Aug 10, 2020

Thus, I'm not even sure now if the scope defined in the BOM contributes to dependency resolution and we should probably remove it.

Sounds good.

May I ask why you would want to change it? If there is a particular issue you are facing, I'll have another look at it.

Nothing in particular. I was just going over how we handle things wrt Maven to reply to http://forum.lwjgl.org/index.php?topic=7062.0 and found it odd that the native artifacts were declared with the compile scope.

TheMrMilchmann added a commit to TheMrMilchmann/lwjgl3 that referenced this issue Aug 17, 2020
Discussed in LWJGL#481.

This only takes care of removing dependency scopes from the BOM for the native artifacts (or more specifically, the workaround is updated to not add them). Adding another workaround to remove them from the "java" artifacts is possible but it's easier to update Gradle to a more recent version where this doesn't scopes are not published for dependency constraints.
TheMrMilchmann added a commit to TheMrMilchmann/lwjgl3 that referenced this issue Aug 17, 2020
This also fixes an issue where the scope of dependency constraints was published in the BOM. (See LWJGL#481)
Spasi pushed a commit that referenced this issue Aug 18, 2020
Discussed in #481.

This only takes care of removing dependency scopes from the BOM for the native artifacts (or more specifically, the workaround is updated to not add them). Adding another workaround to remove them from the "java" artifacts is possible but it's easier to update Gradle to a more recent version where this doesn't scopes are not published for dependency constraints.
Spasi pushed a commit that referenced this issue Aug 18, 2020
This also fixes an issue where the scope of dependency constraints was published in the BOM. (See #481)
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

4 participants