FabricMC / fabric-loom

Gradle build system plugin used to automate the setup of a minecraft mod development environment.
MIT License
233 stars 201 forks source link

Fully support version catalogs #782

Closed bulldog98 closed 1 year ago

bulldog98 commented 1 year ago

At the moment version catalogs are supported nearly everywhere, only place I found were it's not supported is in the loom config.

    mappings libs.yarn

works, but

    mappings loom.layered() {
        officialMojangMappings()
        parchment(libs.parchment)
    }

does not work with corresponding version catalog:

dependencyResolutionManagement {
    versionCatalogs {
        libs {
            library("yarn", "net.fabricmc", "yarn").version("1.19.2+build.1")
            def minecraftVersion = "1.19.2"
            library("parchment", "org.parchmentmc.data", "parchment-${minecraftVersion}").version("2022.08.21@zip")
        }
    }
}
modmuss50 commented 1 year ago

Hi, I have had a go at fixing this here: https://github.com/FabricMC/fabric-loom/pull/785

As far as I can tell versions specified using the version catalog cannot specify the file extension. As parchment is only distributed as zip file, im not too sure if this will work for parchment.

zml2008 commented 1 year ago

The file extension can't be matched directly in the version catalog -- the gradle folks want us to publish module metadata with attributes and in the downstream project, resolve a configuration with variant-aware selection to do some sort of implicit matching.

That's not realistic in a lot of cases in the ecosystem (most notably APs that have a separate annotations artifact distinguished only by classifier, because they haven't bothered to add attributes unique to the annotationProcessor configuration (though that's neither here nor there)), so we have a workaround for classifiers and file types -- I believe in this case you'd do

  mappings(loom.layered {
    officialMojangMappings()
    parchment(variantOf(libs.parchment) {
      artifactType("zip")
    })
  })

and remove the @zip from your version declaration.

Do also be aware that by declaring your version catalogue dependencies in your buildscript, you are losing out on most of the benefits of version catalogs (a simple machine-readable file for your project dependencies, and changing versions without having to wait for your buildscript to recompile) -- it'd be worth your time to actually look at the toml format (or really any other file-based format that plugins may add)

modmuss50 commented 1 year ago

Thanks @zml2008 variantOf is the magic key I was missing. I cannot see it mentioned at all on the docs site... I have updated my PR to use a libs.versions.toml file.

Sadly I think I might have possibly found a Gradle bug in 8.1 nightly, when trying to resolve a variant using a detatched configuration...

Caused by: java.lang.UnsupportedOperationException: Minimal dependencies are immutable.
    at org.gradle.api.internal.artifacts.dependencies.DefaultMinimalDependencyVariant.validateMutation(DefaultMinimalDependencyVariant.java:115)
    at org.gradle.api.internal.artifacts.dependencies.DefaultMinimalDependencyVariant.validateMutation(DefaultMinimalDependencyVariant.java:120)
    at org.gradle.api.internal.artifacts.dependencies.AbstractModuleDependency.setTransitive(AbstractModuleDependency.java:73)
    at org.gradle.api.internal.artifacts.dependencies.AbstractModuleDependency.copyTo(AbstractModuleDependency.java:156)
    at org.gradle.api.internal.artifacts.dependencies.AbstractExternalModuleDependency.copyTo(AbstractExternalModuleDependency.java:56)
    at org.gradle.api.internal.artifacts.dependencies.DefaultMinimalDependencyVariant.copyTo(DefaultMinimalDependencyVariant.java:82)
    at org.gradle.api.internal.artifacts.dependencies.DefaultMinimalDependencyVariant.copy(DefaultMinimalDependencyVariant.java:104)
    at org.gradle.api.internal.artifacts.dependencies.DefaultMinimalDependencyVariant.copy(DefaultMinimalDependencyVariant.java:30)
    at org.gradle.api.internal.artifacts.configurations.DefaultConfigurationContainer.copyAllTo(DefaultConfigurationContainer.java:130)
    at org.gradle.api.internal.artifacts.configurations.DefaultConfigurationContainer.detachedConfiguration(DefaultConfigurationContainer.java:118)
    at org.gradle.api.internal.artifacts.configurations.DefaultConfigurationContainer.detachedConfiguration(DefaultConfigurationContainer.java:52)

I will look into this a bit more later. (Fixed it)

zml2008 commented 1 year ago

Glad that works :)

I believe I got that trick from http://melix.github.io/blog/2021/03/version-catalogs-faq.html#_why_cant_i_use_excludes_or_classifiers - melix did the initial implementation of the whole system, but I guess some info never got fully integrated into the real docs.

raoulvdberge commented 1 year ago

Thanks for this!