FML Clean-up

[haughty copper]

I don't think making that change right now is a good idea without having an idea about how to evolve this API overall

[glad cobalt]

Correct

[chrome mortar]

what change? NDOC'ing fml?

[glad cobalt]

I mean

Making sure that there are no hardcast is a good idea

Even in the current setup

Concepts of caching, or other locating mechanics do not matter

They would require a massive refactor anyway

[haughty copper]

Well to remove the hardcasts, we now have to promote things to API that previously were not, however

[glad cobalt]

Yeah which is fine for now

The amount of API refactoring that will happen is so large

[haughty copper]

I am just hesitant since it might increase the workload to clean it up later

[glad cobalt]

Meh

The workload of these APIs is not really a problem

[haughty copper]

given the use-cases currently do work

[glad cobalt]

The larger workload of the cleanup comes from the fact Maty wants a lazy compile

[split wasp]

Worst case scenario we can presumably get it working with cursed filesystem based fuckery instead, honestly

[haughty copper]

@glad cobalt have you ever considered getting Jij to work at the ML level to allow ML plugins to be JIJ?

Thinking ahead and of neoforge as a mod jar, we likely would want to have FML Jij in it

[glad cobalt]

I thought about it

It is doable

But it would not be pretty!

If we do FML iterative discovery, then we should discover that on ML level too

And then it might be doable

[haughty copper]

Hmmm yeah. I am prototyping some stuff. I.e. I have a working prototype of FML that generates the client jar on-demand using installer-data embedded in a neoforge.jar (which is just in the mods folder)
That works, but if we pursued that, we'd still have to add FML itself to the launcher profile. The only way around that is to have JiJ on the ML-level 🤔

I am trying out having a unified client.jar come out of that, but I still need to verify that the neoforge data+assets correctly override vanilla since y'all said that's why client-extra exists

[plain solar]

Do FML and ML really need to be separate nowadays?

[haughty copper]

I mean define "need"

ML is used by Sponge at the very least

[plain solar]

oh

I am just wondering why we have so many bootstrap loaders

[haughty copper]

If we have neoforge-as-mod-jar, having a version-independent launcher that does the bare minimum to get us to load is nice

since then the installation would just consist of getting the mod-launcher into the launcher profile, and dropping neoforge in the mods folder

ML itself has very few dependencies besides asm... so a few libraries, a different main method and you'd be good to go

updating neoforge/fml would then just consist of replacing the neoforge.jar in mods/, no rerunning installer needed

[plain solar]

I see

[glad cobalt]

Yes

There are many users of ML outside of FML

Hell most of them are even outside of modding

[haughty copper]

The question if we have to cater to them as a project remains, but ML itself isn't too much of a problem, really

I'd personally hide the entire securejar stuff as an impl detail from downstream, though. But that's largely indepenent of the split

[glad cobalt]

Yes we do

The same way Mixin is used downstream outside of Modding

[haughty copper]

As a project we certainly don't have to 😛

[glad cobalt]

Yes and No

[haughty copper]

I'd say at the very least with Sponge it's a give and take, given Mixin

We use Mixin, they use ML. Tit for tat, so to speak 😄

[glad cobalt]

Again, ML is not really an issue

[haughty copper]

There are some things to simplify, but most of the meat is in FML

One thought I had was to use dist-specific entrypoints in ML to make detecting the dist more trivial than it is right now
IDK why mixin for example has to reach into FML to detect the dist

We should have that in ML

All of that would only be possible once ML is modularJPMS-ready though

[plain solar]

as I understand it, the scope of ML should be the same as LaunchWrapper, right?

[haughty copper]

pretty much

[glad cobalt]

No

Not 100%

The combination of BSL and ML is LaunchWrapper

Up to a certain degree

[haughty copper]

BSL is a hack, don't forget that

it would not need to exist if ML was actually JPMS capable

Hm, that is a half-truth. it's less ML and more the consumers of ML that needs to be JPMS-compatible 😄

[glad cobalt]

Correct

I am 100% for it

If we enforce JPMS downstream

But we need to design "encapsulation" breaking mechanics

[plain solar]

what does enforce JPMS mean? everything is a module?

[haughty copper]

Okay the primary reason you cannot put ML itself on the JVM module-path right now is the use of Implementation/Specification version to check for compatibility in downstream projects

Since the concept of Implementation & Specification version do not exist for "normal" JPMS modules

But they do exist for modules created by SJH

[glad cobalt]

Yes

[haughty copper]

well everything is already a module, but we use too much custom plumbing

[plain solar]

Including mods?

[haughty copper]

mods already are modules

[plain solar]

oh yeah

[haughty copper]

but my focus is more on the layers above game... in game you circumvent any encapsulation with AT/Mixin/Coremods anyway

but since those don't affect FML/ML/Libraries, there's potential for encapsulation through modules there

[plain solar]

Why would the tools above game need to do any special hacks (except maybe SJH)

[haughty copper]

they don't

and they don't need SJH either

It's just a property of ML right now that it enforces SecureJar

My next prototype after module-less is probably modules, but vanilla JRE classloaders 😄

[glacial crag]

thoughts on removing ICoreModFile and ICoreModProvider from SPI and directly using CoreMods instead?

(this would also fix the current spi -> coremods -> fml dependency that we have)

[haughty copper]

Hm? I don't see a spi->coremods dependency?

Hm but I think I can see what you mean. There is no extension point there, the SPI is actually the API that coremods implements, but there can only be a single provider

A mod could conceivably call FMLLoader.getCoreModProvider().addCoreMod() at any time, without guarantee that it would do anything.
But the API seems to be available. How do we handle that? Just replace ICoreModProvider with the concrete CoreModProvider class?

[glacial crag]

yeah that's what I would do

[haughty copper]

I'd be okay with that, the API doesn't really get us anything. The coremod lib is exposed as a transitive dependency to mods already (sadly)

[glacial crag]

I anticipate some changes in CoreMods once we get java coremods working, so we could also keep the current thing for now

[haughty copper]

leave it, but if we can hide the actual impl l ibrary from dev that would be nice. not that I have a good idea on how to do that 😄

[glacial crag]

the goal is that we don't need the library anymore once we have java coremods

which in my mind would be loaded from PLUGIN layer jars

[haughty copper]

Hm

I don't know that I'd really do it that way 😄

what kind of classes do coremods need to reach? I'd have said, java.base, coremod api, asm and otherwise fully isolated from anything else?

[glacial crag]

why do we need to isolate them so much? 😄

[haughty copper]

well it's true that they don't really differ too much from ITransformer implementations, I suppose

[glacial crag]

the idea is that they would more or less be ITransformer implementations

[split wasp]

Please dear god don't isolate them any more than is actually necessary. All sorts of useful bytecode transforming libraries people might want to bring in

Besides which, there's really no argument for isolating then beyond just keeping them above all the game or mod level stuff

[glad cobalt]

Because they should not touch or fuck with anything else

And I will be vetoing any PR that lifts that restriction

The point is that they are 100% not allowed to request or load any classes from anywhere

Other then the ones they provide themselves, or are basically ASM and its dependencies

It is part of the architecture

And there 0 reason to change that

They are supposed to be stateless

Adapters of code

They are not supposed to touch anything other then themselves

And the fed in bytecode

[haughty copper]

What is your reasoning for that architecture?

They are less powerful than launch plugins which do NOT have that restriction

You always make this sound like it's a political issue by immediately putting your foot down 😅

[chrome mortar]

yeah there's no "vetoing" this is just shitty intimidation

[glad cobalt]

No there is

This architecutre was specifically chosen

Because we did not have it in the past

And that caused so many fucking untraceable issues

[haughty copper]

Maty let's just explore what is going on, I'd like to know

[glad cobalt]

With people loading classes early

[haughty copper]

What does early mean?

We have a layered class-loader structure now, they can only load up...

[glad cobalt]

It might be possible to loosen

But what would you loosen to?

They are in a completely isolated layer

[haughty copper]

Same principle as launch plugins

[glad cobalt]

On purpose

[haughty copper]

which are in a plugin layer

[glad cobalt]

I mean that seems reasonable

[haughty copper]

essentially we are exporting ITransformer instances

[glad cobalt]

But why?

The point of coremods is that they are stateless self contained systems

Else make a transformer

[haughty copper]

You can't add a transformer from a game content jar right now, I believe

[glad cobalt]

Yeah, but that is the point

Because you get loaded from a game content jar

[haughty copper]

Well I mean even as a JIJ plugin

[glad cobalt]

You have restrictions applied to you

[haughty copper]

no no, JIJ doesn't though

[glad cobalt]

Basically core mods are loaded from a mod jar if I remember properly, like mixin and others

[haughty copper]

Whatever we do for java coremods, the boundary has to be along JAR-files, as in, coremod has to be a separate jar JIJ'd into the game content jar

[glad cobalt]

Seems reasonable yes

[haughty copper]

which then gets loaded above the game layer (and before the game layer even exists)

[glad cobalt]

I mean for Java coremods it makes somewhat sense to allow them to access dependencies

[chrome mortar]

oh i'm just annoyed by how

A team is a group of people with similar goals, and equal weighting in most decision making
is only used when it's beneficial and ignored when it's not lol

[glad cobalt]

Stop the strawman argument, cause taht statement also means that we need consensus to make desicision which we do not have on this topic!

So please discuss this on the content.

This architecture has been put in place in the past because people fucked it up fiercly and violently. It is there because it is a safeguard for us all not because it is usefull or nice to have. It makes our systems a lot easier but also a lot safer

[glad cobalt]

Again in a java coremod environment, I think there might be an argument to loosen it a bit

But to what

There are not many other libraries that makes sense

I mean

Maybe GSON

Or something

[chrome mortar]

no, you can stop with "And I will be vetoing *" cause that's not the attitude this project is meant to have. if you start your sentences with that it's by the book intimidation

you can discuss without threaths

threats is how most FML discussions started and resulted in

[glad cobalt]

Sure, let me state it like this, without reasons to loosen the restrictions, I won't agree with this change, and will request changes to it's justification

[chrome mortar]

i thought we agreed we'd get rid of that?

[glad cobalt]

Does that work for you?

It is not a thread

[chrome mortar]

great thanks

[glad cobalt]

It is a statement of fact

[chrome mortar]

now you can continue the discussion

[glad cobalt]

You know that it means exactly the same thing right?

[chrome mortar]

no, one is absolute "no discussion" and the other is "i'm not a dictator so i'm open to discussion"

[glad cobalt]

A veto is not a "no discussion"

And I specifically stated that i would veto a PR

Not the discussion

I thought that was clear

[glad cobalt]

Back to topic

[chrome mortar]

not sure where people are supposed to get "you can discuss" from that

[glad cobalt]

Sharte the core concept of coremods is that they are isolated from the environment

[glad cobalt]

The fact that I kept writing and arguing

There is still no reason to change it

In my opinion even in java land

They should touch java.base, asm and that it is

They are supposed to be isolated mutators of the code

Not touch random shit

Or load config files

Or mutate the state

As such architecturally there is no reason to lift the isolation

[chrome mortar]

then why do people get "fml should be deleted" from my messages that state that i find fml a mess when i still discuss incremental changes to it that don't necessarily mean throwing it out and rewrite it

[glad cobalt]

As said I could have stated that differently, but can we now get back to topic, and stop discussing my wrong way of writing and hammering this on me

@haughty copper Is there some design or implementation within java coremods that you think is not achieveable with isolation in place?

As in the same isolation that JS Core mods right now have

Besides the fact that they are also isolated through their execution environment

?

[glacial crag]

@Orion the real problem in the old (1.12) design is that there was no classloader isolation AT ALL between transformable code and transforming code

[glad cobalt]

I know

But there is also no argument to undo it

Unless there is something that can not be done with their current architecture in mind

[glacial crag]

So a transformer could directly load game code in the wrong CL and prevent others from transforming it (in 1.12)

I mean, we have the module setup exactly to prevent this sort of issue

A coremod in the PLUGIN layer has no chance whatsoever of loading game classes in the wrong CL

By construction of the modules

[glad cobalt]

Again large part of that restriction comes from the runtime environment JSCoremods are in

I consider Java coremods to be more similar to Transformers so existing in the PLUGIN layer as said above

And with that kind of isolation in mind

[glacial crag]

Not at all, the module system is a fundamental component of the jvm that prevents this sort of CL issue

JS coremods made sense in the 1.16 context where modules were not available, imo

[glad cobalt]

Yeah I know that....... What I am saying is that coremods right now can not have dependencies loaded because of the nashorn runtime it runs in

And indeed the fact that it was designed like way before modules

And I am asking

[glacial crag]

What I have in mind for Java coremods is just loading them via SPI from PLUGIN libraries

[glad cobalt]

With us putting them on PLUGIN, as in using that module layers isolation, is there anythign that a modder could not do, which we would deam reasonable?

[glad cobalt]

Yes I understand that, and that isolation is fine

It is basically JSCoremods minus nashorn

[glacial crag]

Ah but then we're all on the same page

The coremods will be able to access any lib then, since libs are also plugin layer

[glad cobalt]

I assume that is fine

[glacial crag]

(except for GAMELIBRARY jars which will not be accessible)

[glad cobalt]

I see no obvious reason why that would not be fine....

But I also see no reason to lift the restrictions further

[glacial crag]

I mean that's already a massive lifting compared to js coremods 😛

I just got the impression that you wanted more isolation than plugin layer

[glad cobalt]

You are right that the isolation for coremods originated as protection, and that today that protections is given by modules, so the runtime isolation is not needed anymore.

But I am also against lifting restrictions for liftings sake

[glad cobalt]

I would welcome it, if in someway we could really isolate all transformers from each other, to be able to reload and discard them if need be (failing broken transformer scenario)

But I also understand that that is fundamentally more difficult

So I consider that at best a stretch goal

[haughty copper]

On mobile right now so limited ability to follow...

[glacial crag]

I mean that's not really transformer specific

[haughty copper]

Is there a usecase for accessing loadingmodlist to conditionally.enable compatibility transforms?

[glacial crag]

In a stretch world we could recreate all layers, reset FML, and only load forge+mc

[restive stag]

I'm only just skimming the chat since I'm in a meeting right now, but wouldn't it be a lot simpler to initially have no isolation, and add it on at a later point?

[glad cobalt]

No

From experience that is going to get people mad

If we do isolation

We need to set it up right

And do it from the get go

[restive stag]

Gotcha

[glad cobalt]

It is like adding a firewall on whitelist mode after people have deployed 20 apps

That now all break because they try to access something that they previously could

[restive stag]

Yeah, that makes sense; I was just thinking that nobody currently has actually "used" the isolation, so to speak, so they might not even notice that it's missing

[glad cobalt]

Every JSCoremod basically does, because it runs in nashorn

But yeah

No ITransformer ever had any

[glad cobalt]

Maybe, but there might be a discussion topic here if it should not be there

Or at least maybe have a distinction between "priviledged transformers" like for forge, or fml. And "mod transformers"

[haughty copper]

Don't use SJH and use real modular.classloaders instead

[glacial crag]

That would be trivial to do after the fact once we have module reconstruction working

[restive stag]

Is that even possible from the vanilla launcher?

[glacial crag]

If you were to load in safe mode you wouldn't even look at mod jars except forge's

So their transformers wouldn't get loaded at all

[haughty copper]

Of course

[restive stag]

Neat

[haughty copper]

Isn't the core concept just that they can modify bytecode using ASM? 😅

Sorry, I am just getting to the chat history now

[glacial crag]

He means the JS coremods literally

(the CoreMods repo)

Not the overall ASM concept

[haughty copper]

oooh ok

[split wasp]

Question: why is the default layer for JiJ libraries with nothing else specified GAMELIBRARY, not LIBRARY?

The average library that's JiJed doesn't touch vanilla classes. If it does, the developer is aware of FML to some degree and can add that to the manifest

[haughty copper]

There is no GAMELIBRARY Layer

[split wasp]

Erm, whatever the default JiJ one is. It's not LIBRARY

[haughty copper]

There's only BOOT -> [SERVICE|PLUGIN] -> GAME

But without checking, GAMELIBRARY likely goes to GAME while LIBRARY goes to PLUGIN

[split wasp]

Yes that's my question: why are otherwise unspecified libraries that are JiJed sent to GAME not PLUGIN?

[chrome mortar]

that's what the FMLModType is called

[split wasp]

(given that you can JiJ LIBRARY stuff, that you can use LIBRARY stuff from the game but not the other way around, and that most libraries that need to be on GAME will be aware of FML enough to specify GAMELIBRARY themselves but the reverse is not necessarily true)

[glad cobalt]

Because jij is supposed to be used by mods

Anything in the plugin layer can not be touched by the game layer

And by design the game layer is the default layer for ml

So it goes there if nothing is specified

Which is what allows mods and their dependencies to work together

[chrome mortar]

..huh? you mean the other way around?

access works fine top to bottom

[glacial crag]

default is GAMELIBRARY yeah

for jar in jar

for other locators you get an exception if you don't specify anything

[split wasp]

I guess I don't see how treating JiJed stuff as LIBRARY by default would change this. Mods can use LIBRARY layer stuff just fine by default; however, if some mod JiJes a newer version of library X in its normal form, and some plugin layer tool JiJes it but transforms it to load on LIBRARY... the plugin layer thing won't be able to access it if it JiJes an out of date version!

Basically, what is the actual scenario where JiJing stuff as LIBRARY by default causes issues?

The only scenario I can think of is stuff that has to talk to mods or the game, not just be called by it - in which case it is aware of mods and FML and would either have a mods.toml (if it's a mod) or should specify the layer in its manifest

[split wasp]

And yeah this is confusing for me too - plugin layer stuff can be touched by game layer stuff - or at least LIBRARY jars at any rate certainly can be!

[restive stag]

isn't it also the case that by the time the mod/JiJ search is being done, the SERVICE/PLUGIN layer is already completely built and immutable?

at least i vaguely remember that being the case from when I was working on things, which is why I had to split my mod into a bunch of projects to ensure I could appropriately deal with the different module layers

[split wasp]

Language providers can JiJ LIBRARY stuff just fine, and access the JiJed stuff, so I wouldn't think so

Because correct me if I'm wrong but it'd need to be on the plugin layer for a language provider to access it

(as in, GML itself is written in groovy)

[restive stag]

I think I vaguely remember that language providers are loaded on the PLUGIN layer anyway

[split wasp]

And we transform the groovy jar to turn the mod type to LIBRARY at present

[split wasp]

Yeah that would make sense, but JiJ resolution only happens once, right?

So it's happening while that path can still be changed

Unless I've drastically misunderstood the system here which is definitely possible

[restive stag]

i believe that JiJ resolution is done in the SERVICE layer

specifically, in MCML's TransformationServicesHandler::discoverServices, which loads the discovery services and adds them to the SERVICE layer

and it's then done again in the GAME layer

[haughty copper]

you mean PLUGIN?

[split wasp]

So does JiJ discovery happen twice then? That seems like it'd lead to even more issues

I assumed it happened once before language providers and whatnot were loaded

[restive stag]

Possibly, yeah

[split wasp]

For reference, this issue isn't entirely theoretical. It means that language providers or any other thing at that layer have to be extremely careful what stuff they depend on, because anything they depend on is just waiting to explode if a mod also depends on it

[restive stag]

here's the relevant code:

        final var scanResults = this.transformationServicesHandler.initializeTransformationServices(this.argumentHandler, this.environment)
                .stream().collect(Collectors.groupingBy(ITransformationService.Resource::target));
        scanResults.getOrDefault(IModuleLayerManager.Layer.PLUGIN, List.of())
                .stream()
                .<SecureJar>mapMulti((resource, action) -> resource.resources().forEach(action))
                .forEach(np->this.moduleLayerHandler.addToLayer(IModuleLayerManager.Layer.PLUGIN, np));
        this.moduleLayerHandler.buildLayer(IModuleLayerManager.Layer.PLUGIN);
        final var gameResults = this.transformationServicesHandler.triggerScanCompletion(this.moduleLayerHandler)
                .stream().collect(Collectors.groupingBy(ITransformationService.Resource::target));
        final var gameContents = Stream.of(scanResults, gameResults)
                .flatMap(m -> m.getOrDefault(IModuleLayerManager.Layer.GAME, List.of()).stream())
                .<SecureJar>mapMulti((resource, action) -> resource.resources().forEach(action))
                .toList();
        gameContents.forEach(j->this.moduleLayerHandler.addToLayer(IModuleLayerManager.Layer.GAME, j));

[split wasp]

(Heck, I explicitly set the LIBRARY mod type in the artifacts of one of my projects, that isn't really MC specific, for this exact reason)

[restive stag]

so scanResults is a Map<Layer, List<SecureJar>> which contains both PLUGIN and GAME entries

and this is done directly after the SERVICE layer is built

so anything that locates/builds SecureJar objects to be added to the PLUGIN or GAME layers is loaded in the SERVICE layer

[split wasp]

Right, that makes sense to me

[restive stag]

or optionally the BOOT layer if it's provided in the classpath/module path

[split wasp]

The JiJ discovery let's both PLUGIN and GAME stuff be discovered then, as I thought (and as what I see in practice reflects)

[restive stag]

which would suggest to me JarInJarDependencyLocator is in the SERVICE layer

[split wasp]

Which means what I've said is still relevant - there's no reason that the default for JiJed stuff couldn't be LIBRARY

[haughty copper]

Uh while hacking away at NF coremods, this surely can't be needed anymore?

given that function name...

[undone walrus]

yeouch

yes that function does not exist any more

[restive stag]

Okay, I figured it out; JiJ can discover PLUGIN layer jarfiles if the jar is already loaded in the BOOT/SERVICE layer

but mod jars are loaded after the scan has completed

which means that the PLUGIN layer has already been built

though uh, in ModValidator's ctor:

        this.candidatePlugins = lst(modFiles.get(IModFile.Type.LANGPROVIDER));
        this.candidatePlugins.addAll(lst(modFiles.get(IModFile.Type.LIBRARY)));

(candidatePlugins is the List<ModFile> that gets returned and turned into the scanResults map I mentioned earlier)

so from what I can tell, there should be nothing in the code that actually prevents LIBRARY type jars from being loaded by mod files

and those SHOULD be loaded as PLUGIN by ModLauncher

[restive stag]

As for this, it's purely JarInJarDependencyLocator::getDefaultJarModType being set to GAMELIBRARY, since that's likely the best default for most users

given that most mods don't currently specify a FMLModType of MOD, they actually get defaulted to GAMELIBRARY

[split wasp]

Right this works just fine now, that's not the issue. My complaint is that jars without anything are treated as GAMELIBRARY when it's saner to treat them as LIBRARY

[split wasp]

I'm reasonably certain it special cases that in for stuff with a mods.toml and sets it to MOD

Because the normal discovery errors out on stuff without a mods.toml or FMLModType

[split wasp]

Can we make JiJed libraries be LIBRARY instead of GAMELIBRARY by default in a future MC version? Us groovy folks now have an issue where we JiJ a library we use in our language provider as a LIBRARY, by adding an otherwise-absent FMLModType, but someone else JiJes it just normally and theirs is getting loaded instead of ours, and causing an incompatibility with a nasty looking module error. The only things that need to be GAMELIBRARY instead of LIBRARY will be things that reference MC or mod classes, and so already know that Minecraft exists and can define an FMLModType themselves. This would be a breaking change, so we'd have to make it in the next MC version. I can open a PR if folks agree that this is okay to do - this is no longer some theoretical issue though, we actively have an incompatibility with a mod due to it

[glad cobalt]

No

This was discussed a million times now

There are way more users which use JiJ for GAMELIBRARY then LIBRARY

And we want to support that layer by default

However

I think we should support the injection of libraries into a specific layer

But that requires some modifications to the relevant systems to support

[split wasp]

Can you give me an example of something that would be JiJed on GAMELIBRARY where the author wouldn't be aware enough of MC to give it a FMLModType?

Most libraries people use can be JiJed just fine on LIBRARY, and can still be used from game-level code

So for the majority of uses of JiJ this will have no effect

[glad cobalt]

Those don't really exist, but we have as principal to keep the LIBRARY layer as small as possible, so if it is not needed then it should not be in there, is and will be the default way we implement this.

From that follows that the JIJ default layer will always be GAMELAYER, but I agree that it should be easy and trivial tell JiJ to load it in a different layer

[split wasp]

Can you justify this principal, given that it's actively causing issues?

Seems kinda arbitrary

[glad cobalt]

Speed

[split wasp]

How would adding to LIBRARY slow stuff down more than adding to GAMELIBRARY

[glad cobalt]

It is simply faster to lookup classes with as few layers as possible

[eternal wren]

In such a case, JiJ should also determine the layer to load a library in based on the "bottom-most" layer approach IMO
If two mods want to JiJ the same thing one in LIBRARY and one in GAMELIBRARY, LIBRARY should win

[glad cobalt]

Yes

I agree that that should be the behaviour

But unless otherwise specified it should always be GAMELAYER

This is how FML has always done it

[split wasp]

That runs into the issue of services

[glad cobalt]

To allow for the most flexibilty and the lowest hops needed

[eternal wren]

Could you elaborate on that actually?

[glacial crag]

the services got fixed

[split wasp]

Mod A and lang provider B JiJes library C, which loads service X. Mod B also JiJes library D which provides that service

If library C is moved to service because B wants it there, it won't find service X from D

[glad cobalt]

Correct

And that is the proper behaviour

[split wasp]

Which mod A expects it to because with just mod A they're both on GAMELIBRARY

That's the issue

[glad cobalt]

We always had the rules to not allow mixing of the layers

[split wasp]

You can't do what silk proposed without running into this issue

[glad cobalt]

Yeah and that is perfectly fine

[split wasp]

Where mod A breaks when B is present

[glad cobalt]

The design has this issue build in

If you load a library that is not FML Aware

That is always something you need to worry about

The same is for example with shadowing

[split wasp]

It's trivially solved by moving it to LIBRARY by default

[glad cobalt]

Yes

Might be so

[haughty copper]

Isn't that reversed?

[glad cobalt]

But we do not want to do that

[haughty copper]

Library layer is untransformed

[split wasp]

But the point is there's nothing mod A can do about this

[haughty copper]

It's much faster to load it from *PLUGIN/SERVICE rather than GAME

[split wasp]

Mod A will just randomly break

[glad cobalt]

And again, by design we have untill now accepted this limitation

[chrome mortar]

uhm i wouldn't use that as an argument given that gamelibrary classes get transformed

[glad cobalt]

True

[split wasp]

This wasn't a limitation at all before now - this only pops up with the proposed "use the lowest requested layer"

[haughty copper]

I see the point that if the .jar file doesn't declare anything at all and is no mod, why load it in GAME?

[split wasp]

Whereas moving to LIBRARY doesn't have this issue at all, and I've yet to see a solid argument against it besides speed which is looking iffy

[split wasp]

This is basically all I'm saying, yeah - it's far less risky to load it as a LIBRARY, and if it declares nothing at all that's likely where it should go

I get that you've accepted this limitation in the past. I'm saying you no longer should as there's now evidence of it causing unsolvable incompatibilities in production

[glad cobalt]

Yet the problem is that such changes can have a lot of very unforeseen concequences

Just because it fixes your problem

Does not mean that it does not open pandoras box

FML is very hard to test

And it is simply safest option to use GAMELIBRARY

But I am reworking parts of FML anyway right now

[split wasp]

We've shown that that's not the safest

It's just what's currently done

[chrome mortar]

how does making something more restrictive open a pandora's box

[split wasp]

It's inherently unsafe

[chrome mortar]

this is completely backwards

[split wasp]

Furthermore, the alternative proposed solution of loading stuff on the most restrictive requested layer has more potential issues than moving them to LIBRARY

The whole service thing

Which would cause breakages that would be even harder to diagnose than this one

[glad cobalt]

Okey, but you also remove the ability to transform those libraries

Which people might have been doing

You also modified the way the existing libraries are loaded and processed

Because they are in a different layer by nature

Simply stating that you fix A and as such need solution B, does not mean that said solution B does not cause issue C which currently does not exist

I mean it is unlikely that issue C exist in this particular case

but it still needs to be put through testing

And I already agreed to do said testing

[split wasp]

Far more unlikely than issue A which we know exists

And you've yet to propose an alternative solution that doesn't open up far more potential issues that are way more painful to figure out

[glad cobalt]

I would have allowed you to specify the layer, and failed to load it if two mods want to load it in different layers

Same way you can have two different requested version ranges right now

And get a load failure that way

Because as far as I can telöl

Even if I load library A with service spec B in the LIBRARY layer, that will still blow up if their is a mod that provides implementation C

[split wasp]

So mods would have to actively declare which layer they want things on if the things use a service of any form

[glad cobalt]

Because the library consumer of A which loads services B in the LIBRARY layer will never see the implementation C

[split wasp]

That's not the issue

[glad cobalt]

But it is.....

[split wasp]

The issue is a library with an implementation in another library

It's all perfectly safe so long as both libraries are on the same level!

Obviously your example is unresolvable

But this one is! But not via declaring a correct layer for stuff...

Cause they don't need to be on that layer. They just have to be on the same layer as one another

[glad cobalt]

So your argument is that if I have two libraries which are FML unaware, and one of them holds the service spec, and one of the impl, they should always be loade din the same layer correct?

[split wasp]

Yes

That's what you'd want them to do

And you'd want that while mods might need one or the other as a LIBRARY

Because that's the current issue

And you can't rely on libraries declaring services they use, cause not every library is module aware beyond an automatic module name

[glad cobalt]

Realistically does this happen?

[split wasp]

Libraries using a service provided in another library? Yes

Hell, I actively maintain one such library. I declare an FMLModType in it even though it's not MC aware to avoid issues of this type by just locking in where it can show up to begin with

But services as a way to communicate between libraries is kinda the point of services

Currently it doesn't cause an issue because both load on GAMELIBRARY

If you allow one to get dragged higher because someone requests it on LIBRARY, you have an issue, as it wouldn't drag the other with it

[glad cobalt]

As I said I will test it

[split wasp]

This doesn't require testing to tell you it's an issue

[glad cobalt]

It does require testing to solve it

I understand there is an issue

[split wasp]

Hell, you want an example of such a library? Jackson databind. Which. Yeah, Jackson libraries are the ones actively involved here so this would break in our specific case

Jackson databind, and other Jackson libraries, provide services used in Jackson core

GML JiJes some but not all Jackson libraries

Valkyrien Skies JiJes a different set of Jackson libraries

Boom, issues arise - currently, cause they get loaded as GAMELIBRARY when we need them as LIBRARY. With your proposed solution, you'd still have issues as core would be pulled up to LIBRARY but not things that provide services for it

[glad cobalt]

Again beeting a dead horse

I will test it

And report back

[split wasp]

Test what exactly? I've pointed out that this is an issue. We know it's an issue, either at present or with your proposed solution. That doesn't need tested. That's just how services work

[glad cobalt]

Test your solution

[split wasp]

Sorry if I seem impatient about this; I'm just rather annoyed that it took till this actually caused us one of those incompatibilities-that-arent-necessary-but-that-we-cant-fix that we were worried about for this to get taken seriously, which I suppose could be partially just the perception of the situation on my end

[glad cobalt]

It really is

Because you are simply the first to bring it up

And in general I am not oposed to your ideas and solutions

[split wasp]

I brought it up many times before this point and generally got ignored or brushed aside though

[glad cobalt]

I am simply not as quick to accept them on facevalue

And want to get the time and should get the time to do my own investigation and test your ideas

[split wasp]

That's fine. Can you not be dismissive of my ideas right off the bat then?

[glad cobalt]

Well it is the actual first time I have an object case to work with. So it is the first time it got actually stuck on my radar

I first defend the current stance, which might seem dismissive, but you did not provide your examples from the get go

You asked for a change without providing the real reason or examples of what does not work

[split wasp]

The fact that your first response to me bringing this up was literally "No" (#1187879036815417456 message) is telling

[glad cobalt]

And I am not going to do work for free

So my first response to the question "Can you do X?" is always a blanked "No"

Cause you can easily make the PR yourself

And have me review it

[split wasp]

I provided the example from the get go of what the current stance broken

[glad cobalt]

It is a single line change after all

[split wasp]

If your response is "no" when I bring it up here, I'm not making the PR. I've never expected you to do all the work here

Anyways, I'll drop this; probably not productive

[haughty copper]

That is correct. It does disable AT and Mixin into those libraries. Debatable whether that should be encouraged or not, but it's a real consequence.

[split wasp]

With neo on it's own what's loaded as GAMELIBRARY? Do DFU and company go there? Cause as far as I'm aware you can't mixin/AT DFU

And if not, the name GAMELIBRARY is a bit misleading

[glacial crag]

it is a library that gets loaded into the GAME module layer

in that sense, GAMELIBRARY is quite sensible

and no, nothing is in GAMELIBRARY in the latest releases

[split wasp]

Huh, alright, good to know

...wait hmm, does that mean LIBRARY stuff, or whatever that layer that gets that stuff is called, can reference and use DFU? I'm gonna have to abuse that

[glacial crag]

at the moment yes

https://tryitands.ee

(cause I am not 100% sure)

thing is, anything in GAMELIBRARY can use DFU as well 😉

[split wasp]

I will be abusing this heavily. Get ready for BytecodeOps

...well, I'm still waiting on some sort of nicer transformer API I suppose before I go there

[split wasp]

As a follow up to the FMLModType discussion, https://github.com/neoforged/FancyModLoader/pull/96

[split wasp]

Are there cleanup tasks in FML that could use contributors? I would very much like to see #22 become a thing, and am wondering what the major blockers on this are at present

[glacial crag]

oh there's a billion blockers

[split wasp]

Might be worth putting together some sort of roadmap (maybe a github project?) for FML, given the number of different things it has going on

Regardless - in terms of mod module support specifically, are we looking at major architechtural limitaitons or "this system has been around for 5 years, nobody knows what it does, and making this change requires changing it" limitations?

[glacial crag]

I think that an accurate view would be "this system has been around for 5 years, it needs to be cleaned up, and we don't want new extensions with unpredictable impact that might hinder the clean up"

[split wasp]

That's fair. Is there work that needs to be done to clean those up that you could use help with?

[glacial crag]

One of the first steps is the removal of BootstrapLauncher. In theory we could just put everything on the module path

This requires a mixin update however - maybe it could be done now that we're using fabric's fork

[split wasp]

Hmm. What's the issue with mixin at present? Something something service discovery?

[glacial crag]

It relies on a package version check that would fail is normal modular environments

I forgot the details

These package version checks need to be eradicated

[haughty copper]

Yes, implementation and specification version do not exist in JPMS

That they're still there is an enhancement made by SJH but that breaks as soon as you try to load things on the JVMs normal module path

We really need to port the changes over to fabric mixin that I made 🤔 (Well and fix the uses we have ourselves)

[eternal wren]

The version could be pulled from the module, in theory

[haughty copper]

Yes, that was one possible fix, but that does cause issues in dev of these projects

The far easier fix is probably the "old" way of including a version class in API to query it

[buoyant shoal]

I would once again like to request that EventBusSubscriber be moved into an outer class because I don't like how there are three levels of classes in one file and that Mod and EventBusSubscriber don't seem to be functionally similar, other than the fact that Mod also automatically subscribes to events. If people are not opposed to the idea I can open an issue.

[glad cobalt]

EBS is part of FML Java specification

And not part of any other language spec

[mild storm]

That.. doesn't change that it's nested under @Mod ?

[glad cobalt]

Well it kind of does

Because @Mod is the FMLJava annotation

It ties them together

I know it is not really a "thick" connection

Or a good reason

But that is the reason why it is there right now

[jovial pier]

I don't see why the FMLJava spec must be contained within one class

[buoyant shoal]

Both classes should be moved to the javafmlmod package then because right now they are in common, which by your explanations doesn't make sense either

[buoyant shoal]

I agree and this already not the case from what I can tell as Mod ctors are allowed to accept an FMLModContainer, meaning that class is part of the spec (though why that behavior is allowed is also questionable as FMLModContainer has no additional functionality over ModContainer)

[split wasp]

Could also just check both and delegate - like, if the module has a version use that, otherwise try and query the other one. But yeah a general version class in the API is probably the better bet

[split wasp]

And yeah this is the proper solution - this is java. We can use packages for grouping, no need to shove it all in big mega-classes for no reason

[buoyant shoal]

Should I open an issue then?

[split wasp]

I would

That way it doesn't get lost at least

[buoyant shoal]

https://github.com/neoforged/FancyModLoader/issues/101

[chrome mortar]

so.. i tried something for locators https://github.com/Matyrobbrt/FancyModLoader/commit/2580401af01351fe00989a99937720af76afbc69

it will still benefit from an int-based priority system, and some logging but it's a start

cc @glacial crag

[glad cobalt]

If we are already making such breaking changes

Then why not inject the mod file factory via the constructor?

Also I would extract the manifest parsing logic to somewhere else

That locating and manifest management are two different things

[chrome mortar]

it's in a provider..?

[glad cobalt]

Hmm

It still feels weird that you made it public static though

[chrome mortar]

unless you mean moving manifestParser in which case w/e, another class seems pointless

[glad cobalt]

Why did you make it public then?

[chrome mortar]

it needs to be package, it's used in the jij locator and that class both of which are in the same pacjage

[glad cobalt]

Either it is used in multiple locations

And something like a ManifestParser would make more sense

Then move it

[glacial crag]

I don't understand where these stupid files come from

this is such a lambda soup

[plain solar]

maybe running the tests?

[glacial crag]

that or gradle

[grizzled vale]

Lambda soup is tasty, especially with custom methods that are more descriptive than using function etc everywhere

[glad cobalt]

🫛

[errant yew]

ah, the case of the logs folder strikes again

[eternal wren]

If you call that lambda soup, you haven't seem my code, let me tell you

[twilit blaze]

mmm checked exceptions

my favorite garbage feature

[jovial vault]

yeah not really "soup" there, mosr like a little snack

[eternal wren]

I can't share the entire class, but

No well, it's my code, I can

Now that is a lambda soup

[jovial vault]

not sure what to call this, ... lambda lasagna?

[eternal wren]

You still aren't nesting lambdas into lambdas

Look at ExpectationPredicate there lol

[jovial vault]

true, it's not so much stacked on top of each other as next to each other

[eternal wren]

Fun fact, this actually worked first try

[jovial vault]

I was thinking of a peice of code I wrote a bunch of years ago

that was written enterely using continuation-passing style

the code was horrible to debug

because "what comes next" was decided at runtime

[eternal wren]

Giga, are you on PC?

[jovial vault]

yes I am sitting in front of a windows PC and typing on my keyboard, why?

[eternal wren]

Can you give me a link to the file I sent? Discord on mobile doesn't want me to

[eternal wren]

Oof

I wouldn't like that at all...

It's a state machine, but worse

[jovial vault]

https://cdn.discordapp.com/attachments/1187879036815417456/1222326188660490260/MacroDefinitionVerificationTask.java?ex=6615cef9&is=660359f9&hm=0f484ed5e5708a7d414048ca31f41da7a35c16708878d954145764e7691a4359&

[eternal wren]

Thanks

[jovial vault]

that's what clicking on the donwload button gave me :P

[eternal wren]

Yeah, I can't do that on mobile

[jovial vault]

did discord change this? I still see it like this on my phone

I think the betas are running on a different branch from the mainline releases

and have different feature sets

(I use beta, not canary, not release)

[eternal wren]

Yes, but pressing the download button downloads

It doesn't open a page for it

So I cannot get the URL

[jovial vault]

OH

right, it opens in my Edge browser, with a download prompt, but I can't click on the url, and if I cancel it closes the download tab

[glacial crag]

I really like checked exceptions

[tawdry light]

installs Manifold solely for unchecked exceptions

[jovial vault]

I get why checked exceptions exist, but they make the code far too annoying to write

[jaunty jetty]

They also cause a problem when you enter the world of functional programming because Java’s functional APIs have practically no support for them

[split wasp]

Ah, yet another reason I continually wish Java supported higher kinded types.

[glacial crag]

okay the logs/ files come from running the SPI tests with the log4j2.xml file

https://github.com/neoforged/FancyModLoader/pull/107

what is this

why is there a lambda that takes two lambdas and returns a third lambda

that's what I mean by "lambda soup"

from what I can tell the main goal of this refactor is to separate which JARs are found, and how to interpret JARs as mods

[plain solar]

i am not a functional programming expert, but maybe that's the style?

[glad cobalt]

It is

[glacial crag]

the style of what?

[glad cobalt]

It is functional programming

[stable wyvern]

composition

[glacial crag]

it's unreadable soup

[glad cobalt]

Yep

Not really

Just to you

I have grown up with functional apis and it was basically after java the first thing they thought us in uni

So I am used to seeing this

And have apis structured like this

[glacial crag]

that doesn't make it any good

[glad cobalt]

It actually does

[glacial crag]

functional programming has its uses, don't get me wrong

[stable wyvern]

we learned composition patterns in school

[glacial crag]

the goal is readability, FP is just one technique to achieve that

[glad cobalt]

Composition is a proven track record to be the most flexible of api designs

[glacial crag]

I don't care about a "flexible" API if it's an unmaintanable mess because of the lambda soups

(see FML)

[glad cobalt]

What you see here is the composition aspect of fp

Not fp directly

[plain solar]

IMO the main issue with FP in java is the lambda allocations (i think other languages handle it better)

but that's not FP's problem

[glad cobalt]

This is just my opinion: if you do fp in java. You should use your own types

Not function<x,y>

[glacial crag]

it probably depends

[glad cobalt]

It gets you better documentation options for one

And gets you better composition results

[plain solar]

is there a performance benefit from avoiding the generic bouncer method or is that optimized away already

[glacial crag]

forget about performance here

it's fast enough

[plain solar]

it was a general question

[glad cobalt]

There is no performance benefit here

It is a purely architectural benefit

Composition has been proven to be the better oop architecture

But you can't really do composite oop in java, cause there is no real pplymorphism

But you can with lambdas

Because the concept of functional composition is defined differently but has the same outwards facing traits

[junior anvil]

Meanwhile Oracle's pushing DOP for modern Java

[glacial crag]

well the problem here is that there's too many layers of indirection

[junior anvil]

Agreed, that pretty much sums up my personal opinion of modlauncher and fml in general

[glacial crag]

indeed

[glacial crag]

the thing is... sometimes you only locate JARs which you know how to interpret

(e.g. classpath locator)

maybe we should run all of the classpath jars through the mod providers then

and have a way to distinguish between "required" jars (i.e. a mod provider must process them) and "optional" jars (i.e. if nothing processes them then they just get skipped)

(naming is terrible of course)

[chrome mortar]

that should be replaced by a securejar->iconfigurable function

[chrome mortar]

no jar should be skipped

it should display a warning at least if no provider handles ign

[glacial crag]

we probably want #maintenance-talk message before the mod locator changes

(so go review my CoreMods PR )

[glacial crag]

well except for classpath I'd say

[haughty copper]

That method up there is not good for various reasons. One being that using a generic Function interface makes it impossible to find implementors of it

So good luck trying to navigate that codebase for someone who isn't already familiar with it

[buoyant shoal]

I agree

I remember seeing a lot of streams and even though I prefer to never use them, they are not inherently unreadable

[prisma elk]

shadows_of_fire

[Reference to](#modder-1․20․1-support message) #modder-1․20․1-support [➤ ](#modder-1․20․1-support message)FML should probably throw in dev if it finds an invalid loader version decl though

Mar 30, 2024, 10:06 PM

[glacial crag]

ok so maty is still looking into the mod locators, and I have cleaned up mod loading stages

what's next

[chrome mortar]

saying WTF

because that's what the broken files code is making me scream

[glacial crag]

well it's completely broken anyway

(funny haha)

[chrome mortar]

how unfortunate

is IModLocator#isValid even useful

[glacial crag]

is that new in your rework?

of course not

[chrome mortar]

it's in current FML

[glacial crag]

doesn't seem useful, no

[chrome mortar]

ew this is so fucked

[chrome mortar]

https://github.com/Matyrobbrt/FancyModLoader/tree/locator-rewrite there

that should also report fabric/forge mods as warnings

now @glacial crag you can fix the branch cause i don't want to deal with the spotless conflicts

[glacial crag]

sigh OK

[glacial crag]

I think you'll have to open a PR if you want me to push to your repo

[chrome mortar]

there

[glacial crag]

Ok I'll give it a shot soon™️

[glacial crag]

it is done

I had to redo the whole PR... 😒

[glad cobalt]

Sadface

I hope to have a chat with cpw today

So we can clean it up

[glacial crag]

(I mean maty's mod locator rewrite here)

[glacial crag]

that's good

[glacial crag]

which PR?

[glad cobalt]

The document

[glacial crag]

ah even better!

nice

[haughty copper]

That's certainly "a name": JarModsDotTomlModProvider

[haughty copper]

@glacial crag Why are we even returning a Stream from scanCandidates?

Especially given that we call toList() on the return value at the only call-site... locator.scanCandidates().toList()

[chrome mortar]

returning a stream is more convinent for locators

since they'd usually iterate a dir

[haughty copper]

Well in that case, we need to actually close those streams

I don't think terminal operations close streams automatically, so toList will leave the directory stream dangling

Also, only 1 out of 6 locators actually does a directory stream 😅

[chrome mortar]

yes and at least 2 others map a list

3*

[haughty copper]

BTW; some of these locators. I think we discussed this in the past, have we seen actual usage of the MavenDirectory one?

[glacial crag]

any terminal operation should close them

ExplodedDirectoryLocator can be removed, it's superseded by MOD_CLASSES

the maven directory one is also suspicious

[chrome mortar]

tbh I'd move the mod classes logic to explodeddir

instead of unintuitively being in mclocator

[glacial crag]

yeah but they need to be in mclocator for neoforgedev

[haughty copper]

I do not think they do

[glacial crag]

well ok

that seems pretty stupid

[haughty copper]

JarContents.of would be nice for the simple case (new JarContentsBuilder().paths(p).build())

BTW while I am looking at it, the attributes for the generated minecraft mods toml are no longer correct:
mods.set("logoFile", "mcplogo.png");
mods.set("credits", "Mojang, deobfuscated by MCP");
mods.set("authors", "MCP: Searge,ProfMobius,IngisKahn,Fesh0r,ZeuX,R4wk,LexManos,Bspkrs");
mods.set("description", "Minecraft, decompiled and deobfuscated with MCP technology");

[glacial crag]

yeah, maty also requested that

we can do it

[glacial crag]

yeah that's super outdated...

[haughty copper]

https://github.com/McModLauncher/securejarhandler/pull/65

I specifically chose not to use varargs, Tech 😄

[glacial crag]

sure

I don't care 😛

[haughty copper]

The varargs method doesn't work as a method-reference for List.of(...).map(JarContents::of)

And it allows it to be called with zero args, which leads to a runtime error 😅

[glacial crag]

that's pretty sad

[haughty copper]

That's good, it'd hide an array alloc 😄

[glacial crag]

🤷

[errant yew]

and that's why we (Path path, Path... otherPaths)

[haughty copper]

Yes, which also doesn't work well with map

But I've used that pattern myself too

[haughty copper]

What was fml.gameLayerLibraries used for again? when eventbus was still required to be ASM transformed, we had that in there, right?

[glacial crag]

Yes correct

[chrome mortar]

yeet

[glacial crag]

pluginLayerLibraries can be removed too

[haughty copper]

Yes, yeeting

[chrome mortar]

https://tenor.com/view/yeet-trash-seal-dr-dolittle-dolittle-gif-15298225

[glacial crag]

[haughty copper]

I am trying to also get rid of the locators inspecting legacyClassPath directly

I'll have to test my approach, though

Yeet MavenDirectoryLocator too?

[glacial crag]

yess

[glad cobalt]

No

It is actually in use

[haughty copper]

Okay, by what? We might wanna put that in a comment on that class

[glacial crag]

do we pass --fml.mavenRoots anywhere?

I can't find mavenRoots anywhere in NG or NF

[split wasp]

I can't find the option being used anywhere on github, period

[glacial crag]

lol

[split wasp]

Wait that's wrong, there's a few old ones

...all commented out in the code for various launchers, it seems

So if it's used by anything I'm not finding it

[haughty copper]

@glacial crag no idea if it's worth it trying to clean up the use of initArguments / arguments in this cycle

I am half-way through replacing that with a LocatorContext and moving to type-safe keys (think TypeSafeMap) rather than a Map<String, ?>

[glacial crag]

The arguments are not used much, are they?

It's a weird system because of course it can only use arguments that FML defines already

[haughty copper]

It's used more as a way to transport info from the launch handlers and the core to locators

And they contain more info than you'd think

I don't know when you'd use arguments and when the FML Environment

[haughty copper]

But check here for example:
https://github.com/neoforged/FancyModLoader/blob/main/loader/src/main/java/net/neoforged/fml/loading/VersionInfo.java#L12
https://github.com/neoforged/FancyModLoader/blob/main/loader/src/main/java/net/neoforged/fml/loading/moddiscovery/ExplodedDirectoryLocator.java#L73
https://github.com/neoforged/FancyModLoader/blob/main/loader/src/main/java/net/neoforged/fml/loading/moddiscovery/MavenDirectoryLocator.java#L37

Some are set here:
https://github.com/neoforged/FancyModLoader/blob/6a5abe8478f86eabcbd1b3acaafbac7ae6feb004/loader/src/main/java/net/neoforged/fml/loading/FMLServiceProvider.java#L70
Others here:
https://github.com/neoforged/FancyModLoader/blob/main/loader/src/main/java/net/neoforged/fml/loading/FMLLoader.java#L105

[chrome mortar]

maven and exploded dir locators can be yeet

[glad cobalt]

Why are we meeting shit?

Meeting*

Yeeting*

Do these things hurt us?

No

Is there a use for them

Yes

Do we use it ourselves probably no

But up untill now i have seen no justification for yeeting locators!

[haughty copper]

Reduced maintenance burden?

[glad cobalt]

The burden is fucking what?

We have not needed to touch these in literal years

[haughty copper]

Everyone who has to look at this code has to figure out first: "what does this do"

And if it's not used by anyone what exactly is the fucking point of spending that time?

[glad cobalt]

Then document it

[chrome mortar]

because no one used them

[glad cobalt]

Are you sure?

[haughty copper]

Hence I asked you: what's the use case so I can document it

But we haven't actually found one 😛

[chrome mortar]

between yes and yes: yes

[haughty copper]

That's not a guaranteed "no" maty 😛

[glad cobalt]

They can and should be used by launchers that provide a central cache of mods. Using a single argument they can then launch the game with the mods selected without having to copy them

We provide a central implementation for a cache that is structured like a maven

[haughty copper]

mod filenames do not necessarily adhere to maven GAV format

[glad cobalt]

Because that is we where asked for in the past

It is what was asked....

So we included it

Because it was trivial for us to do. It was a standardized format etc

So the locator was added

And just because you can not find shit on github does not mean it is not used. There are a ton of launchers which are not on github.....

[haughty copper]

Yup I was about to comment on that 😛

Not every launcher is OS

[glad cobalt]

We always communicated to launchers: Listen if you want to create a cache centrally were mods are found, and you have a specific format, come to us. We will find a good way to add it

[haughty copper]

Do we know if any launcher actually went ahead with a centralized mod repo?

[glad cobalt]

Yeah it was used very much in the 1.12 to 1.14 days

[haughty copper]

We don't really communicate anything about FML anywhere though, there isn't even a readme for it

[glad cobalt]

Even before that I think

[glad cobalt]

Because all of that happened in the past via Discord / IRC even

The launcher people and lex/cpw came together, and hashed out what was needed

And then it was implemented / provided for them

In such a way that it was easy for us to maintain

And easy for them to use (aka one launch arg, or a launch file)

I am pretty sure there is/was also one that could read from a json file

And load files listed in it

[haughty copper]

not that I have seen

that would indeed be more useful than the list of GAVs though

[glad cobalt]

It might have been yeeted in the past

Or never got out of the PoC

[haughty copper]

Keeping the maven one or not barely matters, really. But keeping essentially dead code since we don't communicate what our external API is, sucks.

So, I'd then actually start writing a README that documents these options we consider public

including this maven one and how it's even used

This one is definitely dead though: ExplodedDirectoryLocator

It reads its list of exploded directories from an argument that no one sets

[glad cobalt]

Yeah i think so

it think that was an old way of including stuff

Instead of from the CP

[haughty copper]

I actually kinda like the idea, honestly. But it's definitely defunct at the moment

[glad cobalt]

Yeah

But where would you use it?

Like what would the use be

[haughty copper]

NG could use it instead of MOD_CLASSES 😅

[glad cobalt]

For the Mven/JsonLocator there is an actual obvious use

[haughty copper]

and pass CLI args instead of an env var

[glad cobalt]

With difficulty.....

[haughty copper]

Since the data model is absolutly identical

[glad cobalt]

Maybe

But in the future we likely don't need either

ModulePatcher supports directories natively

[haughty copper]

public record ExplodedMod(String modid, List<Path> paths) {}

[glad cobalt]

So you can just yeet a set of directorise into a module

[haughty copper]

Look at this record. It models 1:1 what we put into MOD_CLASSES 😄

[glad cobalt]

And call it a days

Yeah for sure

But still

[haughty copper]

I have to read up on module patcher

Currently I am trying to untangle another mess: we read legacyClasspath too much to try and avoid double-loading jars

I'll see if I can replace that with a lookup that just checks if a path is already on one of the layers or their parents

[glad cobalt]

I think we can yeet LCP in the future as well

[haughty copper]

I just hope patch-module is not as useless as the other JVM options

[glad cobalt]

For us, it kind of is.

Because Patch-Module only works on the system / boot layer

Not on our custom layer

[haughty copper]

Yup that's what I thought

I mean ultimately we currently try to solve the problem of grouping a set of seemingly unrelated directories by "mod" they belong to

If we could solve that without additional info just based on the directories, we wouldn't need to pass MOD_CLASSES

[glad cobalt]

The point is

We can extend the patch module system trivially

Because the underlying patcher now has an API

So we can read the patching information for our layer

From anywhere we want

Feed it to the patcher

And done

[haughty copper]

You still need to externally pass the info which directories belong together

[glad cobalt]

Yeah but in the past the patcher was hardcoded to a single property

And not adaptable

[haughty copper]

It sucks that we have to compile that info a priori

that's the entire misery around modSources

[glad cobalt]

There is no really connection, especially in the IDE, between the actual running instance, and gradle

And with the new patcher you don't need to compile it apriori at all

You just need to pass it a map

Well

Yeah

[haughty copper]

It's likely you could heuristically determine that a classes/resources directory belong together

[glad cobalt]

That is an apriori

[haughty copper]

That's literally the same 😄

[glad cobalt]

But you could implement the map interface

[haughty copper]

Just different format

[glad cobalt]

So that it is not compiled apriori

But computed on the fly

[haughty copper]

You cannot compute it on the fly since the information just isn't there unless you apply heuristics

[glad cobalt]

Yes

[haughty copper]

if you have two directories that the IDE puts on the classpath

[glad cobalt]

But that is the problem which you will always have

[haughty copper]

i.e. myproject\build\classes and myproject\build\resources

[glad cobalt]

Because neither the IDE

Nor gradle have any infrastructure for that

[haughty copper]

We could apply a heuristic to group these

I suppose that captures 90% of mods

[glad cobalt]

Maybe

[haughty copper]

But as soon as you start having

project\mod\build\classes
project\mod\build\resources
project\common\build\classes
project\common\build\resources

[glad cobalt]

But in reality it does not matter

[haughty copper]

we cannot really know that the user wants these four directories to go into a single module

[glad cobalt]

Yeah

[haughty copper]

Gradle doesn't know either, to be frank

The user is likely configuring some shadowJar stuff to get a single jar out at the end

[glad cobalt]

Yeah

So in reality an apriori compute is the best UX

It is a bit stupid for us to maintain

But the UX for the user is probably the best

[glacial crag]

worst case they can always request that we add it back

1. remove thing that seems unused and is undocumented
2. someone requests it
3. we add it back with good documentation and a clear explanation of the use case

if nobody uses it we stay with 1. -> profit
if someone complains we end up with better code -> profit

[haughty copper]

Eh I don't care enough about the maven one. It's not terrible

[glad cobalt]

Which is fucking tripple the work for us

[glacial crag]

no it's not - it's just copy/pasting code that we already had and adding documentation

[haughty copper]

I would generalise it over relative directory paths though

[glacial crag]

• it's not you that will do the work it's me 😛

[glad cobalt]

You could just add the documentation

And go home

[glacial crag]

all the dead code in FML makes it harder to understand what is really going on

[glad cobalt]

Or at noen

[glacial crag]

can you link a launcher that uses it then at least?

[glad cobalt]

You are literally yeeting stuff that is at the end of an entire code tree........

You are not reducing complexity with that

Just removing functionality

[haughty copper]

Let's not get hung up on that thing. Let's just document it and that we don't know any users and move on

[glad cobalt]

And again there is 0 proof that it is dead

[haughty copper]

Well Orion it's passing cli arga through several layers.heh

[glad cobalt]

Yeah that is bad, I agree, but right now that is the best infra we have

[haughty copper]

With unchecked casts and string keys

[glad cobalt]

Because ModLauncher nor the Locators themselves have better infra for it

[haughty copper]

That stuff is all in FML actually

[glacial crag]

does (F)ML not have decent joptsimple infra to parse args with strong typing?

[haughty copper]

It does but passes it through the FML arguments map

[glacial crag]

we're cleaning things up - that's why we're having the discussion

[haughty copper]

I already have a lightweight typedmap style alternative...

To at least make this typesafe... Pr later 😅

[glacial crag]

wouldn't it make more sense to pass a context object that contains all the possible args?

[haughty copper]

That's what I have

ILocatorContext

[glacial crag]

ah yes good

that's more or less what I had in mind - the impl can easily be record-backed too

[haughty copper]

But it's still a map with typesafe keys underneath

Since you kinda need that to pass options to custom locators

[glacial crag]

btw I think that Lex removed MavenDirectoryLocator in MCF

[haughty copper]

Other question... What are your thoughts on getting rid of it entirely in favour of using FMLEnvironment

If he did I think we can too

[glacial crag]

https://github.com/MinecraftForge/MinecraftForge/commit/6f9f0f0cfacc74d62be39359d773d5dda220ad73#diff-459b28878dcf6d12fbf72d2987be9bdbbb0e2da780ce56d83358d63d1f1f41f7 removes it completely as far as I can tell

[glad cobalt]

FMLEnvironment is a good idea

I think that laready has a type safe map

Okey then yeet it

[glacial crag]

right now you can get the dist from FMLEnvironment, from FMLLoader and probably a bunch of other places

we should consolidate all the context

you can also get it from ML's Environment using the neoforgespi Environment.Keys.DIST

[haughty copper]

Downside of FMLEnvironment is it's global nature

[glad cobalt]

Why does ML have an Environment, that is what the fuck I am thinking about

[haughty copper]

But that is also an upside hehehe

Yeah the ML Environment and FMLs are easy to mix up

There is also an IEnvironment somewhere

[glacial crag]

that is the ML one

IEnvironment is the ML one

[haughty copper]

But didn't we have one in spi too?

[glacial crag]

Environment is from SPI and can be deleted now that it only contains the dist

it's just a holder class

[haughty copper]

Ah yes so we have Environment, IEnvironment and FMLEnvironment

[jovial vault]

just keep FMLEnvironment, that's what we all use in our code :V

[glacial crag]

in MI I use FMLEnvironment apparently

[jovial vault]

FMLEnvironment.dist.isClient() / FMLEnvironment.dist == Dist.CLIENT is what we tell everyone to use for physical side stuff :P

[glacial crag]

so I would agree with that statement

FMLEnvironment.isClient() helper when? 😛

[haughty copper]

It's just a design question if we want to use this global to pass along cli args too hehehe.

Although they would not be exposed since these keys could be kept internal

For testing it would certainly be nicer to not use a global.

[glad cobalt]

I guess you mean the cleaned args right?

Because I really would not like my access token to be exposed to a random mod.....

Allthough...

Unsure

I think you can already access it today if you really really really tried

[haughty copper]

Yes cleaned

And yes the mod could get it

[chrome mortar]

you know that that's not how it works 😛

[glacial crag]

hmmm?

I thought this was limited to FML's arguments

[haughty copper]

There are various ways of getting the access token if you wanted to within the process

[glad cobalt]

Yeah

[haughty copper]

But yes the args I was taking about are the parsed and cleaned args

[haughty copper]

I have some wild ideas for prototyping things for 1.21, but we should put a pin in what we want to finish for 1.20.5 first

My suggestion:

• Remove all deprecations from ML/SJH/FML/etc.
• Enable Java Core Mods (but keep the current system in parallel, maybe deprecate it?)
• Finish the locators but potentially just stop here

[glacial crag]

sounds good

[glad cobalt]

As an initial step yeah

I would agree

[glacial crag]

deprecations in FML don't really matter - we already cleaned up many of them

[glad cobalt]

We would likely need to touch locators / plugins again later

Bt it is a first step

[haughty copper]

Yes, as I said this would just be what we aim for for 1.20.5

Since we're already at pre1, we likely don't have much time for more

[haughty copper]

@glad cobalt Do you still think we should keep the maven locator if MCF removed it?

I am kinda neutral on the whole thing at this point, I might add

[glad cobalt]

Maybe not. But the concept is right. we should talk to them to figure out how we can help them make such a central cache solution a reality

[haughty copper]

Yeah I can see value in the idea at the very least

I.e. other modding ecosystems like the Bethesda games struggle with similar issues and have developed virtual file systems to work around it (the nexus launcher does something along these lines, I think)

[chrome mortar]

"them"?

[haughty copper]

Launchers

[glacial crag]

The..' what???

How large are their mods?

[chrome mortar]

oh come on, we all know that's not going to happen :P

[haughty copper]

Very large

Gigabytes

At least some of them 😛

[glacial crag]

Jeez

[chrome mortar]

these discussions turn into years long discussion and then more years before they're actually implemented if those discussions actually get somewhere

[haughty copper]

Looking at the hierarchy:
I removed the intermediate abstract class for jar handlers since it was already empty

BuiltinGameLibraryLocator is currently unused (fml.gameLayerLibraries is always empty)

I struggle to find a future use for this

[glad cobalt]

There is not one anymore

[haughty copper]

Yeeting this too: mappingsOption = argumentBuilder.apply("mcpMappings", "MCP Mappings Channel and Version").withRequiredArg().ofType(String.class);

[haughty copper]

@chrome mortar Hm, given the current locator design, if there's an invalid zip-file in mods/ it just crashes completely?

Not that I tried, but I'd assume it does

[haughty copper]

I don't like it, but I also don't really see a way around it

public sealed interface IModFileCandidateLocatorResult {
    record Success(JarContents contents) implements IModFileCandidateLocatorResult {
    }
    record Error(EarlyLoadingException.ExceptionData error) implements IModFileCandidateLocatorResult {
    }
    static IModFileCandidateLocatorResult of(JarContents contents) {
        return new Success(contents);
    }
}

[glacial crag]

you can bump to java 21 btw

(if you haven't yet)

[glacial crag]

the of should really take a path and try/catch it...

[haughty copper]

Yeah that is exactly what I also found out 5 minutes later 😄

static IModFileCandidateLocatorResult result(Path path) {
    try {
        return result(JarContents.of(path));
    } catch (Exception e) {
    }
}
static IModFileCandidateLocatorResult result(List<Path> paths) {
    try {
        return result(JarContents.of(paths));
    } catch (Exception e) {
    }
}
static IModFileCandidateLocatorResult result(JarContents contents) {
    return new IModFileCandidateLocatorResult.Success(contents);
}

The question is what to catch I suppose

I have yet to decide

[haughty copper]

I'll write a few JUnit tests, testing this seems a nightmare

[haughty copper]

libsPath = Path.of(System.getProperty("libraryDirectory", "crazysnowmannonsense/cheezwhizz")); 🤔

[errant yew]

it me, cheesewhiz

[haughty copper]

FMLOnly artifacts currently don't exist as far as I know.

So this is never gonna work, right?
var fmlonly = LibraryFinder.findPathForMaven("net.neoforged.fml", "fmlonly", "", "universal", versionInfo.mcAndFmlVersion());

Note: our installer should already add the mods.toml for the minecraft jar directly into the jar

Probably not for 1.20.5

[errant yew]

they used to exist

(re: fmlonly)

not anymore, though, iirc

[haughty copper]

Yeah, yeeted. Easy enough to reintroduce if we ever really want to.

If our mc jar contained a mods.toml already, I could remove a bit of special case code here hrmpf

[glad cobalt]

Then lets make it so....

Add a patch that adds it I would say?