-
Notifications
You must be signed in to change notification settings - Fork 33
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
Fix: Disambiguate libraries by their module name. #206
Conversation
|
...src/main/java/net/neoforged/fml/loading/moddiscovery/locators/JarInJarDependencyLocator.java
Show resolved
Hide resolved
922a225
to
801adda
Compare
One question is what should happen when a modid and a module name overlap? Should we add some kind of |
Maybe we can do that yeah, just in case. |
801adda
to
84ba6ac
Compare
Ok all done |
That is not true, grouping and selection of the jars, happens solely via the metadata that is included in the json of the containing jar, the identifier information is not actually used for this. What is the current exact problem? |
Selection happens solely via this select method: https://github.com/neoforged/JarJar/blob/b9338af0d631251d5ed6779b14c66bd419a2cf37/selector/src/main/java/net/neoforged/jarjar/selection/JarSelector.java#L181-L218 that directly selects via the |
if (modFile.getModInfos().isEmpty()) { | ||
// Prefix to ensure this cannot collide with any true mod ID. | ||
return "library:" + modFile.getModFileInfo().moduleName(); | ||
} | ||
|
||
return modFile.getModInfos().stream().map(IModInfo::getModId).collect(Collectors.joining()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're already fixing up this logic, can we fix this logic here too, by joining them with something that can't be in a mod ID, like +
or ,
? The current logic would treat a jar containing the mods foo
and bar
as identical to a jar containing the mod foobar
, which it is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of scope IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other option is to take just the first mod ID -- which may be more sensible, as that is what becomes the module name. That said -- is there a reason we don't just use the module name in general here? After all, it'll be unique for different mods and for other stuff, but the same between multiple mods that are the "same" mod or whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to merge this, looks fine for now.
🚀 This PR has been released as FancyModLoader version |
MixinExtras is a library rather than a mod, at the request of the team, but this means that JiJ'ed versions are not correctly grouped since the current logic uses the filename.
This allows users to JiJ new stable versions of mixinextras.
There is another bug with the module name detection for beta versions, but I will fix that separately by setting an
Automatic-Module-Name
.