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

Respect the archivesName #46

Open
wants to merge 5 commits into
base: 1.21
Choose a base branch
from
Open

Respect the archivesName #46

wants to merge 5 commits into from

Conversation

uecasm
Copy link

@uecasm uecasm commented Mar 30, 2024

I'm not a gradle expert, so I'm not sure if there's a better way to do this, but at least with userdev 7.0.97 (as is referenced here), changes to the base.archivesName don't actually do anything without a line like this.

@Shadows-of-Fire Shadows-of-Fire added the enhancement New feature or request label Mar 30, 2024
@Shadows-of-Fire
Copy link

Default maven path before this change:
com\example\examplemod\MDK-main\1.0.0\MDK-main-1.0.0.jar
Default maven path after this change:
com\example\examplemod\examplemod\1.0.0\examplemod-1.0.0.jar

Copy link
Member

@sciwhiz12 sciwhiz12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the Gradle User Guide, the default value of artifactId is based off Project#getName() rather than the base archive name.

Should we consider setting the project name to the mod ID in addition to (or perhaps in replacement of) this change?

build.gradle Show resolved Hide resolved
@Shadows-of-Fire
Copy link

I wasn't sure if there would be other implications of fiddling with the project name (does it change the name displayed in an IDE?)

Also, the project name is seldom the modid, rather it is usually the mod name without spaces (due to repository naming schemes).

@sciwhiz12
Copy link
Member

(does it change the name displayed in an IDE?)

It does; if I recall correctly, IDEA displays the folder name and then in parentheses the project name (or was it the other way around?).

Also, the project name is seldom the modid, rather it is usually the mod name without spaces (due to repository naming schemes).

Good point. I think, though, the more pertinent issue (as I've just remembered) is a problem with case-insensitive file-systems, where a case difference between the mod ID and the repository/folder name might cause issues. (I think I remember having experienced this issue on Windows when I used Eclipse, and possibly with IDEA.)

@uecasm
Copy link
Author

uecasm commented Mar 31, 2024

The motivating case was when I was trying to set the archivesName like so:

base {
    archivesName = "MyModName-${minecraft_version}"
}

This is, I think, fairly typical, where the project id/name is the mod id (or folder name?) but the archive name is set to a capitalized/de-spaced friendly mod name, with Minecraft version included (so that it's part of the filename but not part of the actual mod version). Not sure if the MDK should do that by default too, but a random sampling of curseforge mods suggests it's commonplace.

Side note: the Forge MDK published a sources jar by default, while the NeoForge MDK does not. That's probably not important for most mods but they're nice to have for any mod that gets addons made for it (which are the type to end up published in mavens in the first place), so perhaps also might be helpful by default anyway?

@sciwhiz12
Copy link
Member

I think I can agree with those changes: creating/publishing the sources JAR by default (via java.withSourcesJar()), and adding the Minecraft version to the base archives name.

@uecasm
Copy link
Author

uecasm commented Apr 1, 2024

How's this look? I changed the archive name to use the project name by default (rather than the mod id, which may or may not be the same), mostly because the mod_name can't be assumed to be space-free. But mod authors can still edit it as needed to get the desired artifact name.

build.gradle Outdated Show resolved Hide resolved
Comment on lines +127 to +129
java {
withSourcesJar()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this above the comment (which is for the publishling block), and add its own comment explaining that this enables the creation of the -sources JAR.

build.gradle Outdated
@@ -113,7 +113,7 @@ tasks.withType(ProcessResources).configureEach {
minecraft_version : minecraft_version, minecraft_version_range: minecraft_version_range,
neo_version : neo_version, neo_version_range: neo_version_range,
loader_version_range: loader_version_range,
mod_id : mod_id, mod_name: mod_name, mod_license: mod_license, mod_version: mod_version,
mod_id : mod_id, mod_name: mod_name, file_name: file_name, mod_license: mod_license, mod_version: mod_version,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to supply the file name as a property to the mods.toml properties expansion. Note that mod_group_id is also not provided as a property here.

build.gradle Show resolved Hide resolved
@@ -14,7 +14,7 @@ repositories {
}

base {
archivesName = mod_id
archivesName = "${file_name}-${minecraft_version}"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR has set the base archives name to be the name of the published module -- however, you're now publishing the artifact at com.example.examplemod:ExampleMod-1.20.4. By convention, module names are lowercase, so this is not great. The inclusion of the minecraft version in the module name is also not optimal - it means that someone could depend on both ExampleMod-1.20.4 and, say, ExampleMod-1.20.2 and gradle would just pull in both -- whereas publishing every version of a mod under the same module name wouldn't allow this to happen.

The other solution to the latter problem is the use of capabilities but that is way to complicated for the MDK.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost every mod file I've ever seen has published using WordCase names.

Copy link

@lukebemish lukebemish Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and that's fine for CF/MR publishing but a bad pattern for maven publishing (which is all this block is for) because it's the opposite of what is done in the wider gradle and maven world. Let's set a good example instead of propagating a problematic pattern. The use of the MC version in the module name is an even bigger issue, as it can lead to issues during dependency resolution and getting two copies of a module.

@@ -29,6 +29,8 @@ loader_version_range=[2,)
mod_id=examplemod
# The human-readable display name for the mod.
mod_name=Example Mod
# The mod filename, usually the mod_id or the mod_name WithoutSpaces
file_name=ExampleMod

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above: the file name can be this for the local file and that's fine, but for the published module (the bit in the publishing block) it should be a normal, conventional module name -- in this case, probably com.example.examplemod:examplemod. A simple way to do this is to set the rootProject.name in the settings.gradle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants