FML Clean-up

[haughty copper]

and add the dummy deps to that

[glacial crag]

Are we leaking the runtimeClasspath maybe?

Apparently that's what we're already doing

[glad cobalt]

We are not

[haughty copper]

Wait, so does this work when you runClient? Or what? 😄

[glad cobalt]

As far as I can see it should

[haughty copper]

We should try that

[glad cobalt]

Do we have an example for that?

[haughty copper]

Also as an aside: not having a named config to represent the runtime classpath of runs sucks

Because we cannot inspect it using gradlew dependencies

[glad cobalt]

runtimeClasspath

[haughty copper]

It is extended though

[glad cobalt]

Yeah

I am considering making it one

Because I want to use it in tests

So it will likely come

[haughty copper]

But first step is to inspec gradlew dependencies for the normal runtimeClasspath, yes

[glad cobalt]

No

The way the error manifests is always in the IDE, whether eclipse or IDEA

Both of them track classpath contents via the path of the jar

Explicitly not the GAV

So because of that you get into a situation where when they construct a runs classpath for running through them, they add both elements to the classpath

This has been known for years

This is also not just limited to Minecraft or NeoForge

[haughty copper]

It is very much possible, yes. But we should confirm this and document it in the locator if that's the case for the hacks we use

[glad cobalt]

This same problem happens with all dependencies

[haughty copper]

But TBH the IJ dependency model actually should support this Orion

not leaking a dependency from module A to module B

[glad cobalt]

You would think so right

Since it knows it is a gradle dep

But it is does not accross module boundaries

That is the fucking problem

At least it was last time

And because multi project builds create multiple distinct modules in IDEA you still run into the issue

[glad cobalt]

That is actually exactly what it is not doing

It is not leaking

That is the point

If it where leaking

It would use the dependency from module A in module B

But it is not

Yet because your run uses them both in the classpath

[haughty copper]

I just meant that the project model itself in IJ should support this case. It's still possible that their Gradle sync is unable to properly convert it.
In any case, we should test this first with gradlew runClient to check whether this issue is already present in the Gradle project model or not.

If all else fails, we reintroduce reading the MC dependency from legacy classpath and add a large comment explaining why the fuck we do that

[glad cobalt]

If somebody gives me a repo project

I will do it right now

Because this is a really high prio for me to fix

[haughty copper]

I don't have one right now either

@restive stag Is the project where you ran into this public?

[restive stag]

Not yet, unfortunately - I'm working on it 😅

[glad cobalt]

Should a dummy workspace with two MDKs not work?

[haughty copper]

It might

[glad cobalt]

Still in meetings

Will check once I am done

[haughty copper]

Same for meetings 😄

[glacial crag]

Lol what. Apparently my suggestion of using two configurations worked 😂

Unfortunately I am at work and can't test anything before like 7 pm

[glad cobalt]

That seems utterly weird, because that is what NG7 does internally........

Exactly for that reason

Why would extending runtimeClasspath from a configuration

[glacial crag]

Yeah I'm confused as well now

[glad cobalt]

Vs

Adding the dependency directly to runtimeClasspath make any difference?

Because your suggesting is this:

// Sets up a dependency configuration called 'localRuntime'.
// This configuration should be used instead of 'runtimeOnly' to declare
// a dependency that will be present for runtime testing but that is
// "optional", meaning it will not be pulled by dependents of this mod.
configurations {
    runtimeClasspath.extendsFrom localRuntime
}

dependencies {
    compileOnly "net.neoforged.neoforge:neoforge:XXX"
    localRuntime "net.neoforged.neoforge:neoforge:XXX"
}

While NG does by default:

[glacial crag]

Well I'm surprised it even works

[glad cobalt]

dependencies {
    compileOnly "net.neoforged.neoforge:neoforge:XXX"
    runtimeClasspath "net.neoforged.neoforge:neoforge:XXX"
}

[glad cobalt]

It works, but what I don't understand is why it fixes it

That is what makes like 0 sense to me

[glacial crag]

Wait you just tested and it works??

[glad cobalt]

The top one works

The bottom one not

The bottom one is what NG7 does

When you do implementation "net.neoforged.neoforge:neoforge:XXX"

[glacial crag]

Weird

[glad cobalt]

It removes it from implementation

And adds it to compileOnly and runtimeClasspath

Yeah

This whole idea of doing this

Came from luke

To prevent them from showing up in maven poms and gradle metadata

[glacial crag]

Well I think it's the correct end behavior

[glad cobalt]

It is

For sure

Hence me implementing it based on his suggestion

What I don't understand is why it works with the intermediary config

But not with the direct add

The end result is the same

[chrome mortar]

uhm the getsuperconfigs is very suspicious

[glad cobalt]

Can you expand that statement?

[chrome mortar]

runtimeClasspath extends from implementation

[glacial crag]

Where?

[glad cobalt]

ConfigurationUtils

[chrome mortar]

the logic is completely backwards and explains why tech's code works

[glad cobalt]

I am going to write some unit tests

Sitting in teamchat

If somebody wants to figure this out with me

[haughty copper]

I'll be metting all day

[chrome mortar]

the issue is getsuperconfigs, that doesn't need or make sense to exist

[glad cobalt]

It does.......

I just wrote 4 unit tests to test it

And it works perfectly fine

Not sure what you think it is supposed to do

[chrome mortar]

to test "what"?

broken behaviour?

[glad cobalt]

Its intended behaviour

[chrome mortar]

so it leaking deps is intended behaviour?

[glad cobalt]

To find the runtime and compile classpath configurations

[glad cobalt]

Can you stop with these strawman arguments......

[chrome mortar]

it's not s strawman

that's what it does

[glad cobalt]

The methods in configuration utils are finding the correct configurations

[chrome mortar]

configurations {
  myCustomConfig
  runtimeClasspath.extendsFrom myCustomConfig
}

dependencies {
  myCustomConfig 'net.neoforge:neoforge:'
}

now replace myCustomConfig with implementation since that's how implementation basically looks like

and that explains why it's leaking deps

runtimeClasspath extends from impl, and you're getting all the "super"s (which is wrong, since gradle is in reverse here) and adding the deps to them too

the dep ends up on impl again

[glad cobalt]

Yet it does not

Because the dep does not show up in the gradle metadata

Nor in the maven pom

So it can't be in implementation

@chrome mortar You can run the tests in this class: https://github.com/neoforged/NeoGradle/blob/fix/multi-project-leaking/common/src/test/java/net/neoforged/gradle/common/util/ConfigurationUtilsTest.java#L19 and see that you are wrong

The assertions clearly show that the implementation configuration is not returned

So unless I am missing something in the tests (which is possible), your hypothesis that this is caused by this section of the code is not correct

And that means that the replacement configurations for "implementation" are properly selected

[glad cobalt]

Correct, what this part of the code is doing, is take in a configuration and a project. Then take all sourcesets from the project, and take from that sourceset the runtimeclasspath configuration, and then finding all configurations that this runtimeclasspath extendsfrom, and then check if the input configuration is part of that extends from set

So as far as I can see it works as designed

And as planned

I can see how you think it works in the wrong direction

But it does not

[chrome mortar]

well replacement seems to let implementation remain with a random unspecificed dep

[glad cobalt]

How did you test this?

[chrome mortar]

dependencies {
      implementation "net.neoforged:neoforge:${neo_version}"
}

and ran ./gradlew dependencies

[glad cobalt]

Wtf

Why would it do that

I am going to do some digging

But should be testable

[glad cobalt]

Ah that is the generation task

Which is technically fine

Because that has no dependencies that can leak

Still it likely should not be there I think

Let me see if I can write some tests that verify that

[split wasp]

Did the system for targeting classes for transformation via the constant pool ever go anywhere?

I've been eyeing that idea for a while now; would honestly be quite useful

[plain solar]

no, and without designing the existing transformers to not parse every class, it's of no use

the current big issue I encountered is that Mixin's ML hook parses every class unconditionally, and there is no obvious way I see to find out if a given class needs transformation

[split wasp]

Mixin only does stuff to classes with mixins targetting them though?

And it knows that list, so it should just be able to only target those classes

[plain solar]

yes in theory it should be possible to fix

I just didn't see any obvious way to, given my lack of knowledge of Mixin internals

[split wasp]

Also it's not of no use even then -- folks doing their own custom stuff could add their own transformers using it, that will be future-proofed against whenever we figure out how to fix the mixin one

But... mixin internals. Lovely. Those are always a true nightmare to poke. Worth looking into I guess to see what we can figure out

[plain solar]

Llama might know a way

[glacial crag]

this is really weird

this suggests that it will only handle classes if one of the "post processors" requests it

[plain solar]

One of them always votes for it, forget which

I can check

MixinTransformationHandler

any non-empty class is always voted for

[split wasp]

With the new changes, noticing some leaking of internal stuff. The LoadingModList leaks internal classes (namely, ModFileInfo, which isn't strictly internal but leaks ModFile which is marked as internal), leading to strange IDE warnings

See #modder-support-1․20․6 message

[haughty copper]

I am not terribly happy with the entire handover from FMLLoader->LoadingModList->ModLoader, to be frank

[buoyant shoal]

Sometimes I wish there was a single place to check mods like you can do on Fabric

[glacial crag]

Well that person is "abusing" a newly-introduced API 😛

[split wasp]

Hmm? LoadingModList isn't new, and you've been able to grab stuff from it for a while, right?

Wasn't it always the recommended way for inspecting mod stuff at weirdly early times?

[glacial crag]

That new method is new

[glacial crag]

It is but it doesn't really have a well-defined API

[split wasp]

Of course

[split wasp]

Still not sure what thing here is new -- getModFiles() is not, nor is getFile(), and getting the loading mod list was easy before with methods that don't seem to be marked "internal". The bigger issue is that there's no good well-defined API here, and that @ApiStatus.Internal is used inconsistently throughout the codebase.

Note that no method should have a return or parameter type that involves an @ApiStatus.Internal class unless that method it, itself, @ApiStatus.Internal!

[glacial crag]

It might be a different method than what I have in mind then

But yes FML doesn't have a particularly clean design

[chrome mortar]

3 unused exceptions

[glacial crag]

[glacial crag]

@haughty copper you broke the FML build 😄

you got sniped by another change hehe

[haughty copper]

F

on it

For a sec I thought to myself "hmmm do I need to rebase this? it's still green...."

[chrome mortar]

I think it's the first time it's happened

gg you deserve a medal /lh

[haughty copper]

@glacial crag Reclick please: https://github.com/neoforged/FancyModLoader/pull/157

[glacial crag]

done

[haughty copper]

ty

[haughty copper]

Alright, so current musings on startup:

https://github.com/neoforged/FancyModLoader/tree/experiments/startup

JavaAgent is kinda awesome

Has perf-impact when transforming, but we can use it without a transformer and then it has no impact. That still allows us to completely remove all --add-opens args and nicely set up modules as we want at runtime, even for java.lang

For tests run from IDE, where we may not be able to control the javaagent arg easily at the moment, we can use dynamic attach until that gets yeeted from the JRE in likely the next LTS (Gradle has until then to figure their shit out)

But I don't think that'll be much of a problem since the enterprise world relies on Mockito and Powermock and I believe both use the Bytebuddy dynamic attach agent at the moment.

[plain solar]

@haughty copper is this intended for dev or prod

I feel like attaching the java agent in prod will be messy

[haughty copper]

Why?

[plain solar]

how will you get the path to the jar?

[haughty copper]

Same as with the modulepath?

Our installer already makes those assumptions for the module path

[plain solar]

hmm k

[haughty copper]

Here's a snippet of the Vanilla Launcher profile

"jvm": [
   // ...
   "-p",
   "${library_directory}/cpw/mods/securejarhandler/2.1.24/securejarhandler-2.1.24.jar...
   // ...
]
// ...
"libraries": [
{
      "name": "cpw.mods:securejarhandler:2.1.24@jar",
      "downloads": {
        "artifact": {
          // ...
          "path": "cpw/mods/securejarhandler/2.1.24/securejarhandler-2.1.24.jar"
        }
      }
    },
}

The same approach should work for passing "-javaagent"

[haughty copper]

That's a new one.... TinyFD (or is it Windows?) complaining about quotes in my error message... what

[eternal wren]

TinyFD

[haughty copper]

Oh huh. That check is nowhere to be found on GH, but I noticed that mirror is extremely outdated

[jovial vault]

[haughty copper]

The checks are present in the version on sourceforge, which seems to be the primary source

[jovial vault]

(but at the same time not surprising that it does stuff like that -- making crossplatform dialogs is a deep can of worms)

[eternal wren]

Correct

Though, why TinyFD over NFD?

[haughty copper]

Well it's piping stuff into a shell apparently? What the...

What is NFD?

[jovial vault]

lwjgl uses tinyfd

https://github.com/mlabbe/nativefiledialog

[haughty copper]

Yeah since this is for FML Startup fatal error reporting in case not even the early startup window can be shown/found

I don't have a lot of options

I could theoretically just use AWT, to be honest

[jovial vault]

doesn't lwjgl prevent awt from being used?

[eternal wren]

Nope

[haughty copper]

Since... if earlydisplay didn't even start and we're about to crash, I really don't need to care about messing up OpenGL for later 😄

[jovial vault]

in macos specifically

[haughty copper]

I thought it's more the other way around

[eternal wren]

LWJGL uses a patched GLFW that avoids the problem

IIRC

[haughty copper]

if you use AWT you create an off-thread message loop

[jovial vault]

I think lwjgl intentionally breaks awt by forcing headless mode to be set

[haughty copper]

You can just unset that

[eternal wren]

Lwjgl has both

[jovial vault]

but maybe that only matters if you have actually initialized opengl XD

[haughty copper]

I think that's it

[jovial vault]

it does? huh.

[haughty copper]

lwjgl might, but does MC ship with both

[eternal wren]

Oh, this is still within MC's context

Nevermind then

[jovial vault]

no nfd in the list

[eternal wren]

MC doesn't use it, yes

[haughty copper]

Well yeah, but I think AWT is the better option, when I think about it

[jovial vault]

remember to test in mac

[eternal wren]

Though what does MC even use TinyFD for?

[jovial vault]

or have someone test in mac

[haughty copper]

Since assuming the FML main method is called and none of the MC libraries are on the path, I can only use AWT to show an error anyway

[jovial vault]

nothing, apparently

[jovial vault]

oh I probably don't have sources downloaded here at work

yep.

grumbles about jetbrains

more like nobrains :V

[eternal wren]

Somehow my brain went to MessageBoxW

[jovial vault]

I wish showing a dialog box in a crossplatform way was actually as easy as it is in windows

I mean, windows - easy. other platforms - not so easy

[haughty copper]

Yup

[eternal wren]

Windows W

[glacial crag]

is it not easy on mac?

on linux: good luck 😄

[eternal wren]

Does Linux even have such a concept?

[haughty copper]

Better it has many many many such concepts

[glacial crag]

which display server and window system do you want to support lol

[eternal wren]

So it doesn't

[jovial vault]

I believe in mac you need to have a GUI initialized in order to show a messagebox

in linux you have to pick a GUI framework and hope that whatever DE you use will work

oh it's apparently not that bad in mac. https://gist.github.com/brendandahl/9061968

unless they deleted that API and replaced it with something else, which is possible

[haughty copper]

tinyfd is certainly not using that

[eternal wren]

Uh

Mac has a C++ API?

[haughty copper]

It instead seems to build an applyscript file and executes that

[eternal wren]

Aren't they all on ObjC?

[jovial vault]

yesn't. there's bindings people have written

[haughty copper]

https://en.wikipedia.org/wiki/Core_Foundation sys it's C

[jovial vault]

so it's basically doing "this" instead https://apple.stackexchange.com/questions/73290/display-dialog-from-command-line-like-xmessage-does

anyhow

[eternal wren]

Oh, so it's the low level upon which ObjC runs

[jovial vault]

point stands

windows? ez. just call MessageBox* function

[haughty copper]

tinyfd seems to try to support OSX<9

[jovial vault]

mac? well maybe you can call API, maybe you need to execute a dynamically created shell script

[haughty copper]

Shame on them not also having Win 3.11 support

[jovial vault]

linux? well you either depend on some random gui framework which may be 1000x bigger than your app, or run a shell command that calls xmessage

[haughty copper]

only xmessage?

what if you don't have that

[eternal wren]

Run a shell command that installs xmessage by using apt

[haughty copper]

https://github.com/native-toolkit/libtinyfiledialogs/blob/master/tinyfiledialogs.c#L3448

[eternal wren]

If you don't have apt, then install it via whatever way you do to install apy

[jovial vault]

oh god that's WORSE than I thought

[eternal wren]

Imagine using C APIs

[haughty copper]

There's a lot more checks than that

[eternal wren]

In a C application

[jovial vault]

thoguht I had pasted the others

[haughty copper]

// TinyFD refuses to let us use quotes
message = message.replace('"', '`');
message = message.replace('\'', '`');

Well, I'll just go with that for the time being!

[jovial vault]

but I only pasted one lol

[haughty copper]

until we know we can actually use AWT

If we can use AWT, we can also use Swing...

then we can make actually useful error dialogs 😄

Also earlydisplay still has a text-QR code embedded in it that's unused

leads to tinyurl.com/forgetest 😄

Leading to https://blog.minecraftforge.net/personal/matyrobbrt/optifine-alternatives/ 😄

@chrome mortar blogger!

[eternal wren]

if (getOs() != MAC) {
  displayWithAwt();
} else {
  tinyfd("An error occurred. Upgrade to a better OS to know what happened");
}

👍

But yeah, IIRC LWJGL now ships a patched GLFW that doesn't conflict with AWT on Mac

It should be tested, but I remember discussions about it on LWJGLCord with Spasi

[haughty copper]

I didn't see them shipping separate glfw artifacts though

patched ones, Im ean

[eternal wren]

https://github.com/LWJGL-CI/glfw/tree/macos-async

Don't ask me about the artifact name

[haughty copper]

I mean, is that just the GLFW used by LWJGL then?

I don't wanna buy a mac to test this shit 😄

[eternal wren]

LWJGL has its own fork of GLFW and various other libraries, yes

But I cannot confirm that it is the same file that is run on MC

You'd have to check the dylib name

And I can't ATM

[haughty copper]

I mean, they just pull in the LWJGL natives, so if LWJGL patched it, MC benefits

[eternal wren]

I don't know if LWJGL patches it in the main artifact or requires a different one

Hence why

[prisma elk]

thesilkminer

[Reference to](#1187879036815417456 message) #1187879036815417456 [➤ ](#1187879036815417456 message)Don't ask me about the artifact name

Jun 28, 2024, 11:55 AM

[eternal wren]

I write everything Windows exclusive, so I didn't interact with that

I simply remember a discussion and lengthy debugging talks

[haughty copper]

Hm yeah looking at that patch btw, that is not something we'd want to trigger 😄

Apparently it does a cross-thread dispatch then which I'd wager decreases perf

[glacial crag]

I mean... you can have fun with objc_sendMsg but I wouldn't recommend it 😛

[eternal wren]

It's fine

It could be worse

I've seen worse

[haughty copper]

Ugh I am still so annoyed at us shoehorning modules into this

The basic APIs we'd really want just aren't exposed by the JDK 😐

[glacial crag]

which ones?

[haughty copper]

Patch Modules

[glacial crag]

ah yes that one

[haughty copper]

And generally the infrastructure around constructing automatic modules from directories is... annoyingly internal

Cue me copy pasting the list of reserved java keywords from the JDK, just as SJH did 😄

The entire ecosystem is really... not there. STILL not there.

[glacial crag]

does every jar not have an automatic module name yet?

[haughty copper]

The problem here is in development I get the disjointed folders

Just like with mods

I.e. when developing the startup code itself, reloading "itself" into a module layer is more difficult than it seems

Since you have to first re-assemble the disjointed directories, then you have to scan which packages it includes to build the module descriptor

If you had access to the patch module infra, I suppose we could instead construct the module from the first folder and patch the resource folder into that module

I suppose a maintainer vote about dropping modules is not gonna fly (I doubt most people even care, though). So I'll suffer through it 😄

Urgh this might actually be the first approach that works at finding these directories by module in dev:

[haughty copper]

Urghslhslhlsh: https://github.com/neoforged/FancyModLoader/blob/startup-experiments/startup/src/main/java/net/neoforged/fmlstartup/LayeredDirectoryModuleReader.java#L19

[errant yew]

// We do not want to deal with that. The spec says use forward slashes.

surprised you put the contains check before the isEmpty check

[errant yew]

https://github.com/neoforged/FancyModLoader/blob/main/loader/src/main/java/net/neoforged/fml/util/ObfuscationReflectionHelper.java still mentions SRG and MCP

we should probably rename that to ReflectionHelper

[haughty copper]

Yeah true, we might also consider if as a generic util, NF itself is the better place. 🤔

[haughty copper]

Good point, actually 🤔

[glad cobalt]

@haughty copper There is ways to use the patch stuff

It now has public constructors

So you can create your own patching infrastructure and pass that to layers, modules and classloaders where needed

[haughty copper]

Well the constructor is public, but it's still all in jdk.internal 😄

[glad cobalt]

Some of it yeah

[haughty copper]

But /sigh. At least I think I found a light-weight solution to making it viable for developing on FML or any other of the service-level projects in development

Where it will re-assemble a module out of the Gradle classes+resources directory

My approach is: I place a file in src/main/resources: <module_name>.resourceroot
Which contains a list of required Java packages that the module must have.

When I don't find required module X at startup, I look for <module_name>.resourceroot in all directories on the CP.
If I find it, I mark that directory down as the "resources" dir for module X.
Then I scan the other CP directories for Java packages and use the one that contains all packages required for module X as the "classes" dir for module X.

That way this works out of the box with Gradle, IJ, whatever and requires no further buildscript interaction

[glad cobalt]

Would an incremental AP not work better here?

[haughty copper]

Hm, it's possible. The resourceroot file doesn't have to contain all packages of the module. Just what you need to identify the classes directory securely

I still need to scan that to get a full package list for the ModuleDescriptor. Sadly this is before SJH is even loaded

[glacial crag]

No AP

[haughty copper]

So far this actually seems to work reasonably well.
In module-land, a package can only exist in one module anyway, so it seems to be sufficient for our use-case.
The use-case here is only for service-level modules in development, which never span more than one classes/resources directory (unlike mods)

[chrome mortar]

@haughty copper https://github.com/neoforged/FancyModLoader/pull/170#discussion_r1659276890 you touched en_us and ignored all others 😛

all other langs still use {0} etc for the broken files

[haughty copper]

If the keys didnt change, would crowdin not update the placeholders? 🤔

[chrome mortar]

you need to take into consideration files that actually had it translated (are there any?)

but really, if you've already done a python

you can do more 😛

[haughty copper]

I already deleted the python 😄
But this was just a regexp replace anyway

[haughty copper]

Ugh now I also had to replicate the service-file scanning. I feel the SJH Pain 😄

TBF, if our boot/startup projects that we wanted to pull as source into the dev-env of FML had module-info.class, I'd not have to do any of that

[haughty copper]

Gnahahahaha .... fuck 😄

I am now getting earlydisplay... in a child classloader. And when I then try to show the tinyfd dialog back in my actual main method..............:

Exception in thread "main" java.lang.UnsatisfiedLinkError: Native Library C:\Users\Sebastian\AppData\Local\Temp\lwjgl_Sebastian\3.3.3+5\x64\lwjgl.dll already loaded in another classloader
at java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:167)
at java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:139)

We also forgot updating ATs in FML itself: https://github.com/neoforged/FancyModLoader/blob/main/gradle.properties#L4C33-L4C34

[haughty copper]

god tinyfd sucks so much

it attaches its message box on windows to the "ForegroundWindow", even if that is from another process

if I kill the process via IntelliJ while it is showing the message box, it locks mouse input to IJ, forcing me to restart it...

[glad cobalt]

That sounds like garbage

[haughty copper]

It is

[glad cobalt]

So tinyfd a no go then?

Is there an alternative?

[haughty copper]

Well multiple, I suppose:
If not on MacOS, we can just use Swing
When on MacOS, the caveat above doesn't apply since it launches a damn applescript process to show a messagebox...

Before LWJGL starts, we can also use Swing on MacOS

I think ultimately we will want earlydisplay to show such errors (after it has started), eliminating the need for tinyfd once earlydisplay runs
before earlydisplay runs, we can use Swing

And now I managed to break MDG 😄

Failed to pre-load artifact 'project :modlauncher' from path 'C:\Neo Forged\modlauncher\build\libs\modlauncher-11.0.5.jar': java.lang.IllegalArgumentException: Malformed Maven coordinate: project :modlauncher

[glad cobalt]

Oeps

[haughty copper]

And now gradleutils is having a bit of a configuration cache fit again

Once more. Maddening. Gradle has the information I need, but choses not to expose it at all.
I'd love to get at that file path...................

Hmmm, I suppose I can get at the path, but not anything else. Oh well

[haughty copper]

Okay well, I finally got past that.... Now I can try building runs in FML that actually simulate userdev and production

I'll also have to see how I can simulate neodev

[haughty copper]

Gradle is driving me up a wall again...

Execution failed for task ':startup:dependencies'.

Could not resolve all dependencies for configuration ':startup:userdevTestRuntimeClasspath'.
java.util.ConcurrentModificationException (no error message)
Deep within Gradle when resolving dependencies ❤️

[haughty copper]

Aight, hit my first REAL issue

FileSystemProviders suuuuuuuuuuuck 😄

[glad cobalt]

😛

I have been refactoring NG to use that everywhere XD

[haughty copper]

Well NG is forced to not use modules anyway 😛

[glad cobalt]

?

[haughty copper]

I am talking about NIO ! 😄

[glad cobalt]

Ooooh those

[haughty copper]

UnionFS / JarJarFS

[glad cobalt]

For fucks sake gradle

[haughty copper]

I know

It infects your brain 😄

[glad cobalt]

There is an actually object that is named that

Why do they suck?

I have never had an issue with them

[haughty copper]

Well obviously not the providers themselves

It's that the JDK only allows them to be on the system classloader

[glad cobalt]

No i mean their api

Well no

That is a concequence of us loading them so early

They can technically be anywhere

[haughty copper]

Well "kinda", they need to be there for tha Path->URI roundtrip to work, no?

[glad cobalt]

If I remember correctly

Yes, they need to be loaded by the time that the first conversion is done

the problem is that it caches that

[haughty copper]

Yes, not only that

it specifically does this:

ServiceLoader<FileSystemProvider> sl = ServiceLoader
    .load(FileSystemProvider.class, ClassLoader.getSystemClassLoader());

I am now getting to the actual mod-loading part and it is only calling loadInstalledProviders for the first time, after I have already set up our module layer nicely

But it still loads them all from the app classpath

I'll see what I can do I suppose 😄

[glad cobalt]

It is technically just a static field

You can just you know, override/add to it

[haughty copper]

Yeah I know, I was hoping to not have to resolve to haxxx

[glad cobalt]

Blame JDK

[haughty copper]

For sure

I don't see a reason why they did this

[glad cobalt]

Performance probably

But in reality they should have done this in a classloader aware way

Where this cachable information is stored in some metadata attached to the classloader

Because else you get into hot water

But that opens a whole host of other problems

[haughty copper]

Maybe some day I can convince you that we don't have to use Modules 😄
But I know, that day is not today 😄

[glad cobalt]

You really can't

I have seen them all

[haughty copper]

Eh, if we kept the constraints, it'd really be easy

[glad cobalt]

Also the hacks that both Forge and Fabric apply will get ripped from Java eventually

[haughty copper]

The constraints being: no default package, packages must not overlap between jars

Which hacks?

Child-first class-loaders aren't hacks

[glad cobalt]

The implicit reliance on the URL classloader mechanics

[haughty copper]

??? What do you mean

[glacial crag]

What is hacky about fabric loader?

[haughty copper]

I don't know the details of the fabric one, but they don't have to use add-opens 😄

[glad cobalt]

Old FML (from before 1.12) relies heavily on implied URL classloader mechanics, so does the new MCF mechanic that they introduced after the split.

Fabric might not actually depend on those, but last time I checked, they still use URL classloaders

[haughty copper]

What mechanics though?

So here's what I know:

Ultimately all you need is a child-first class-loader that only loads child-first those classes that are subject to transformation, right?

Where the problem is: how do you know, which classes are subject to transformation

[glad cobalt]

That behaviour is however only implicitly implemented in URL classloaders last I checked

[haughty copper]

No?

[glad cobalt]

The module classloaders are the only ones that have that explicitly defined

[haughty copper]

Child-first is the default concept for any plugin classloader ever

[glad cobalt]

Sure, but not by the JDK

[haughty copper]

The JDK isn't involved here

They just specify the API in the classloader

[glad cobalt]

Sure it is...

[haughty copper]

Or are you suggesting they'd introduce a constraint on a class by-name only ever being able to be loaded once per JVM?

That would break so much in this eco-system (Gradle COUGH) that there would be riot in the streets 😄

[glad cobalt]

No, but modules partially enfore that no?

It is not a full enfore sure

But a partial

[haughty copper]

Well, we don't break that enforcement though

We only ever load classes in that child-first loader

if they have not been loaded before

from the app classloader

[glad cobalt]

That is however not defined behaviour.......

[haughty copper]

WDYM? It is actually a preqreuisite for us and fabric both

[glacial crag]

What the fuck

Classloaders are very clearly specified

[haughty copper]

The JVM by the way, would not even care

But we want to avoid this anyway

[glacial crag]

Using child classloaders is an extremely standard Java feature

[haughty copper]

Personally, I understand btw, that extra care has to be taken to avoid loading the class from the wrong loader, although that is even possible today in dev

[glad cobalt]

No they are not...... That is the whole point. And Thread can override any classloader, if modules is not specified. And force load any class any which way.

[haughty copper]

today, as in using our module loaders

Orion they can do that with modules too

[glad cobalt]

No they can't

[haughty copper]

Sure

Everyone can reach for ClassLoader.getSystemClassloader 😄

[glacial crag]

Sorry you're just so wrong I can't continue discussing this

[haughty copper]

As we see with the JDK doing it

Tech you're misunderstanding the point

[glad cobalt]

Yeah you can get the system classloader allright

But that does not matter for the transformation system

[haughty copper]

He means that the hierarchy has a package->module mapping

[glad cobalt]

Exactly

[haughty copper]

I think if we ever decided to go back to non-modular classloading

[glad cobalt]

Modules are the only system in the JVM that has the link Package -> Classloader

[haughty copper]

we would absolutly keep that constraint, by the way

[glad cobalt]

And which are enforced by the JVM and JDK themselves

[glad cobalt]

Except that there is no way to enforce that

[haughty copper]

because it also trivializes the filtering in the transforming classloader

sure there is

[glad cobalt]

How?

[haughty copper]

we are loading the jars ourselves

we still index the packages they contain

and we would use that information to set up the filter in the transforming class-loader

to make the decision: does this package need to be transformed: yes / no

[glad cobalt]

Sure, we have done that since 1.0.0

But you can trivially easily brick it

[haughty copper]

Well kinda but we haven't enforced it on a jar to jar basis

[glad cobalt]

Simply by asking the parent of the transforming classloader to load the class

[haughty copper]

Well, in dev at least, I'd have a solution to prevent it from being bricked 😄

[glad cobalt]

Which is not posisble in Module land

Because that module is not part of the layer and there is no way to get it there

[haughty copper]

Well you can get to the system CL but it'd be outside of your hierarchy

It also only ever matters in dev anyway

[glad cobalt]

The systemcl can not load any random mod jar

The mods are not on the systems boot layer

[haughty copper]

You can add them via instrumentation 😄

[glad cobalt]

Seriously?

[haughty copper]

Yes

It has a method to extend the app classloader classpath post-factum

Sadly, only for non-modular jars

Or rather, only adding to the unnamed module, to be precise

[glad cobalt]

Your argument against modules (which have been declared as the way forward by the JVM developers themselves) is that somebody might add instrumentation?

[haughty copper]

But it doesn't matter. What I was getting at

In dev, we can actually check this (also using instrumentation)

[glad cobalt]

instrumentation is not ment as a runtime tool

[haughty copper]

no Orion, my argument against modules is

that the JDK obviously did not have our use case in mind 😄

[glad cobalt]

It clearly did

[haughty copper]

You cannot seriously think that

[glad cobalt]

Sure i can

[haughty copper]

when they keep all the shit private that we actually have to use

when they do not offer a way to --add-opens / --add-export / --patch-module

any of the module layers a JVM user might construct

the entire mechanism ignores our use case completely

it does not have parity with non-module-land

But considering we might even be able to work around that

the biggest hurdles are actually still not the JDK itself

but rather the tooling around it, which to this day is atrocious

I have not seen IntelliJ make any moves towards more control over the module CP, sadly

[glad cobalt]

?

IDEA 2017, 2018 and 2019 introduced massive support for it

[haughty copper]

Try to get IJ to add some things to the module-path, and not others when you work on a mod project and start anything 😄

[glad cobalt]

Including runs, parsing and managing of the modulepath

[glad cobalt]

That is the issue!

The JVM does not want you to do it half

[haughty copper]

The JVM supports that fine

[glad cobalt]

Neither does any IDEA

No it does not

[haughty copper]

It's all well defined

Oh please Orion

[glad cobalt]

It shoe horns it into the unnamed module

[haughty copper]

The unnamed module

is a well defined concept

Are you suggesting they'll remove it?

If you really are suggesting non-modular support is somehow going away in the JVM, please provide some actual hard evidence for it. Because then I can start sharpening my pitchforks at work since no one is using modules 😄

The only space where I've actually seen modules be used is in the client-side desktop app space that want to ship their embedded JRE

Well, except IJ 😅

[glad cobalt]

Probably eventually.

To enable legacy access mode you have to supply --illegal-access=permit so that unnamed modules can work fully as if they are in classpath mode, for one

Second is the fact that unnamed modules can not access java.se.ee, at all. Period.

Not anytime soon though

unnamed modules, are a hack/bridge

To make migration easier

But I personally don't think they are intended to exist for ever, no

[haughty copper]

So. No evidence then...

[glad cobalt]

It is not direct evidence

Everybody would climb the trees if Oracle said: -cp is getting yeeted

Use modules

[haughty copper]

It's none at all

There's no JEP, no proposal from them, nothing

Yet you keep claiming this

It's really riling me up, hrmpf

[glad cobalt]

The original JEP that introduces this, clearly states that unnamed modules are not the same as the normal legacy classpath

[haughty copper]

Yes, and?

The legacy classpath is gone

Everything is in some form of module, I'd say

Even if it's the unnamed one

[glad cobalt]

Since when does it do that?

Because I can't find any documentation on any of it

If you don't use the module path

[haughty copper]

Since the JEP?

[glad cobalt]

No JEP 261 does not specify that

[haughty copper]

I mean there's a reason for having to --add-opens to unnamed

[glad cobalt]

Yeah if you have a module path

[haughty copper]

But, I'll check

You always have a module path

[glad cobalt]

If not there is no mention of it being needed

[haughty copper]

the runtime itself is always modular

But yeah, here in my experiment some random class loaded via -cp

Honestly, that was always my understanding of it.

The migration is that anything on the -cp turns up in the unnamed module of the app classloader

With some special rules of upgrading it, as far as I remember

[glad cobalt]

It might be since J17

Because they enabled Enhanced Enforcement there

[haughty copper]

No that was just for encapsulation as far as I know

I mean I can try, hold on

[glad cobalt]

It might have alwys been the case unsure

[haughty copper]

I am pretty sure it was

[glad cobalt]

But that would mean that any JDK<8 lib that performed reflective access was at risk

[haughty copper]

Well yes, and they were

Since J16 😛

Because that was the change

Strong enforcement of the actual constraints

Also, by the way. From the JDKs perspective, modules were still a success even if almost no one uses them directly.
Since it did allowe them to compartmentalize the JDK itself.

[glad cobalt]

Yes

Fully agreed

Okey

They won't remove it

Most likely

[haughty copper]

You should really reconsider if we need it, but I'll continue gritting my teeth at working around this problem with NIO 😛

[glad cobalt]

I really think we do

I am also a modder that thinks we should enforce it better

And make more use of it

[haughty copper]

In the game layer?

[glad cobalt]

Yeah

[haughty copper]

we offer asm + mixins + ATs

I don't really think it goes with the spirit

library layer is a bit of a different story

[glad cobalt]

I know I am not in the majority with this opinion: My mod code is not to be touched. I spend a lot of time creating APIs that basically allow any kind of modification of behavior

Poeple mixing in, ASMing, or ATIng my shit should be kicked

But I am not in the majority here

[haughty copper]

As long as we offer those low-level transforms, any enforcement is kinda moot

[glad cobalt]

No....

We can trivially filter the stuff we offer through transformation based on the module data

Aka: No open package -> No transform

[haughty copper]

Well, since we fixed EventBus needing asm

I suppose you could

[glad cobalt]

EventBus only transformed event classes

Never callers

[haughty copper]

Yes but if you had an event class in your mod

that would have meant it would have needed to be part of the transform

[glad cobalt]

Yes, but that would really always be an API IMHO

Or would you create an Event

[haughty copper]

Anyway that is gone

[glad cobalt]

Only to use it internally?

[haughty copper]

so no point in discussing that further

[glad cobalt]

Yeah

[haughty copper]

There's @OnlyIn, but we want to be rid of that anyway

I actually think from a startup performance point of view, opting-out of being transformed would be nice lol 😄

but as you said, riots! think of the riots!

[glad cobalt]

My point is this: I think the module system has one major advantage over normal CP:
Its enforcement mechanis, defining what loads what and how. It is enforced by the JVM it self

[haughty copper]

I mean in non-module-land

[glad cobalt]

Yeah I think we can gain a decent amount of performance there

But indeed

[haughty copper]

it'd just be a classloader as it is now 😄

[glad cobalt]

People would not just swing one pitch fork

They would swign like a million of them

[haughty copper]

I'd have to opt in for people to transform AE2 anyway

Addons do so much shit I can't account for

[haughty copper]

Hm. I did discover something very interesting.

What I didn't realize is that you can kinda add modules to the system class-loader after the fact too 😄

[haughty copper]

Now this is some magic. With no module-path given to the JVM, and no reflection/unsafe

[glacial crag]

This is strictly about deep reflection, it's not related to unnamed modules

Unnamed modules are indeed a well-defined concept, used to load from from the classpath

[haughty copper]

Anyway it is staggering how much we have to fight the JDK and the tools to get to use modules, while getting very little in return right now. I suppose the API feels cool though

[glad cobalt]

How did you do that?

[glacial crag]

I mean that's basically the issue, no? Someone thought it would be cool even though it makes our impl a lot more complicated

Unfortunately it doesn't look like a nice cost/benefit analysis was performed

[glacial crag]

And no instrumentation?

[haughty copper]

DefineModuleWithBlah and just pass it the system CL in the CL lookup function

That does add the layer to the CL

[glad cobalt]

Yeah

[haughty copper]

But not the modules which should be good enough for the NIO FS

[glad cobalt]

Yeah I think so

Well maybe

Depends

Does the ServiceLoader ask for the resources

[haughty copper]

Only annoyance is that union is in SJH which depends on ASM

[glad cobalt]

Then lets split that off

[haughty copper]

No it works with the SL if you load the provider class before

[glacial crag]

Any reason not to do the same for everything?

[haughty copper]

Resources don't work correctly if the module is not defined in the CL

We could reflect addModule

Then it would work yes

[glacial crag]

I think that we already do something like that in SJH

[haughty copper]

But for NIO FS it will work without any add opens or reflection

[glad cobalt]

I am trying to understand the problem you are trying to solve

[haughty copper]

Except we still need instrumentation to add opens for the hacks SJH is using itself

[haughty copper]

Arg free startup

No module path

[glad cobalt]

Why?

[haughty copper]

But keep it working with current APIs

[glad cobalt]

Again Why? Why arg free startup.

[haughty copper]

Because module path is a blocker for supporting unit tests without hacks piled on hacks in the IDE

LCP is independent of that

[glad cobalt]

You mean: run unit tests with the ide

Instead of run unit tests with gradle through the ide

?

To be specific?

[haughty copper]

Eclipse does not properly support that at all

Running through Gradle and getting in IDE reporting

[glad cobalt]

Eclipse is indeed and will always indeed be a darkhorse

[haughty copper]

You just get console output

[glad cobalt]

Does it sync test information at all

[haughty copper]

But it is possible to do it

[glad cobalt]

IDEA at least looks it up

[haughty copper]

Not for Gradle no

[glad cobalt]

And asks for the information from it

That is sad

[glacial crag]

Can't we configure eclipse to add these vm args?

[haughty copper]

I keep telling you it's not worth it doing modules but you can't claim I am not trying to make it work 😅😅

[haughty copper]

No

[glad cobalt]

I wonder why we are still supporting an IDE, that is lacking major features with respect to integration to the

[haughty copper]

We can't even do it for IJ properly since it affects all IDE subprojects

[glad cobalt]

No, eclipse really has no way to interact with at all in this area

[haughty copper]

IJ does not support our half module setup either

[glad cobalt]

True

[haughty copper]

Eclipse will.be getting JUnit run templates

Next version

[glad cobalt]

Question: When rebuilding the entire module stack ourselves, as far as I know FS lookup is the only major blocker

[haughty copper]

But those are too broadly scoped too, like the ones in IJ

[haughty copper]

So far yes

[glacial crag]

Another problem is that we can't reuse the --patch-module infra

Not a blocker

[glad cobalt]

Because it uses the system classloader

Correct?

[glacial crag]

Jist a major annoyance

[haughty copper]

Yes

I am on the go but will try to get it to work with the modules we have and no reflect

I need to pull ASM into that layer but it has no resources either

And yes at some point we could pull out union into a dep free jar

But I am trying this without actually breaking current compat

I plan to test it with ATM10

[haughty copper]

Sadly we will never be able to just use the vanilla launchers {classpath} token and pass it to --module-path

[glad cobalt]

Why not?

What is the major blocker there?

[glacial crag]

it probably contains the MC jar

[haughty copper]

Ses

Yes

The obfuscated one

[glad cobalt]

For fucking fucks sake

Well we still probably could

[haughty copper]

And that would block the three unobfuscated packages

[glad cobalt]

We just need to write our own classpath block

And not inherit the config from vanilla

Not particularlly clean

But possible

[glacial crag]

we can't use the net.minecraft package in a module on a child layer though

[haughty copper]

No he means not using the token hehrhe

[glad cobalt]

No i mean tha

But the launcher token {classpath} is currently the combination of our classpath array entries and the vanilla version launcher profile

Because we explicitly extend it

So it combines the classpaths of both

[haughty copper]

It will always add a client jar to it afaik

[glad cobalt]

And sets that as the token contents

Me and covers dug into that for a while

[haughty copper]

Not tested what happens if you don't define any client artifact in the profile

[glad cobalt]

And that is not something we noticed

Let me see if the internet in the ICE is good enough to dig up that research for you

And the answer is no

Internet is too bad

[haughty copper]

It's okay, we might want it to out the client there later for neo as a mod

It would save us from.having to look for.it on disk

When I am home I will test that method of extending the system CL module path using reflection into addModule on all JDKs

[glacial crag]

that doesn't work with unit tests in eclipse, right?

[haughty copper]

Why not?

[glacial crag]

"reflection"

[haughty copper]

Wdym?

[glacial crag]

no --add-opens

[haughty copper]

Paired with instrumentation obviously 😅

[glacial crag]

how will you attach the agent?

[haughty copper]

Bytebuddy. Same as mockito

As a fallback if ours is not attached

[glacial crag]

that will break once dynamic attach is disabled

I am working under the assumption that we have absolutely no control over jvm args

[haughty copper]

Guess what happens when java breaks mockito hehehehe

[glacial crag]

(which is a ridiculous assumption IMO)

[glacial crag]

people get told to add it as a jvm arg and it's all good?

[haughty copper]

We really do not right now but IJ will have to budge for the java agent

There is already a Gradle issue for it

[glacial crag]

IntelliJ doesn't give control over jvm args either? 🤨

[haughty copper]

But we can always add.the agent in the JUnit defaults

Since it does nothing if unused

But adding our argfiles breaks tests in non mdg projects

[glacial crag]

yeah

[haughty copper]

I mean we are lucky in this case since mockito relies on it and so many paying gradle and IJ customers use it

There is a grade issue for it too

[glacial crag]

yes, that's why it's generally a good idea to use what the rest of the ecosystem uses

[haughty copper]

you gotta love IJ!

You create a module-info.java

Then delete it again

And it still tries to start it in module mode 😄

[glad cobalt]

lol

[haughty copper]

Fuck me, even after closing / reopening the project

[glad cobalt]

Does it store it in like the module.xml

Or something stupid?

Or where does it pull it from

Cause it did not do that in the past

[haughty copper]

Oh wait, it did that because of Automatic-Module-Name, what the fuck

If that is present in src/main/resources/MANIFEST.MF

If it's just present in the jar task it don't care

[glad cobalt]

Yeah

[haughty copper]

But it's completely stupid

It doesn't put anything on the module path

it just invokes the main method using --module

which fails

[glad cobalt]

lol

I wonder what the reasoning is behind that

Knowing that these IDEs are build for profit

There must be something behind that concept

[haughty copper]

I really don't think modules are used much, so not a lot of testing on that end

The most I've seen it used is JavaFX

And that also didn't work great for me in IJ

So... in Java 9, this works:

Method loadModule = systemCl.getClass().getMethod("loadModule", ModuleReference.class);
return moduleReference -> {
    try {
        loadModule.invoke(systemCl, moduleReference);
    } catch (IllegalAccessException | InvocationTargetException e) {
        throw new RuntimeException(e);
    }
};

Only 13 more JDKs to test 😄

(and yeah that needs either an add-opens or Instrumentation to be accessible). Or I suppose, Unsafe? Idk

[glacial crag]

Unsafe is going away in a few years

[haughty copper]

Yes instrumentation also does the trick, really

Full method:

private static Consumer<ModuleReference> getModuleAdder(Instrumentation instrumentation, ClassLoader systemCl) throws Exception {
    instrumentation.redefineModule(
            systemCl.getClass().getModule(),
            Set.of(),
            Map.of(),
            Map.of(
                    systemCl.getClass().getPackageName(), Set.of(MpExtensionTest.class.getModule())
            ),
            Set.of(),
            Map.of()
    );
    Method loadModule = systemCl.getClass().getMethod("loadModule", ModuleReference.class);
    return moduleReference -> {
        try {
            loadModule.invoke(systemCl, moduleReference);
        } catch (IllegalAccessException | InvocationTargetException e) {
            throw new RuntimeException(e);
        }
    };
}

My JavaAgent:

public class MpAgent {
    static Instrumentation instrumentation;
    public static void premain(String arguments, Instrumentation instrumentation) {
        MpAgent.instrumentation = instrumentation;
    }
    public static void agentmain(String arguments, Instrumentation instrumentation) {
        MpAgent.instrumentation = instrumentation;
    }
}

I don't actually need agentmain per se, but eh

Then I do this in my main:

Instrumentation instrumentation = MpAgent.instrumentation;
if (instrumentation == null) {
    System.err.println("No agent present. Using dynamic attach.");
    instrumentation = ByteBuddyAgent.install();
}

[haughty copper]

Hehehehe, setup-java on GH can't get java 9

[haughty copper]

Okay, I just used gradle toolchains instead.

I am happy to report that at least this reflected loadModule method works from java 9 to 22

[haughty copper]

Oh come oooooooon our neoforge.mods.toml seriously does not specify a version?

[haughty copper]

made https://github.com/neoforged/NeoForge/pull/1207 and will special case that placeholder in the meantime

Also this seems really outdated:

[haughty copper]

@glad cobalt Found yet another reason why we're fucked with modules 😄

[glad cobalt]

?

[haughty copper]

Vanilla Minecraft don't care that LWJGL native modules all have the same module name

[glad cobalt]

Vanilla Minecraft also does not download all the native modules

[haughty copper]

since it's a legitimate configuration to just put ARM64, x64 and x86 native jars on the CP and launch

[glad cobalt]

It has actuall selectors for that

[haughty copper]

No

It doesn'T

I assumed that until 5 minutes ago too

But there is only an OS filter, not an architecture filter

[glad cobalt]

Does it have them for all 3 architectures then?

[haughty copper]

Yes

[glad cobalt]

How has this worked then in the past?

[haughty copper]

And BSL implicitly takes care of that by auto-merging modules of the same name

[glad cobalt]

Yeah

[haughty copper]

Which I found quite surprising

Since putting those three on the module-path instead just crashes

[glad cobalt]

That was indeed the reason it did that

We should ping a problem causer

[haughty copper]

I thought that might be a bug

but LWJGL says: nope

Design decision 😄

[glad cobalt]

Cause putting all those three natives on the CP seems like a majorly bad idea

[haughty copper]

https://github.com/LWJGL/lwjgl3/issues/856

It's not, really

I think LWJGL here is.... weird

Because their code explicitly handles this and the packaged natives are in different subpackages

[glad cobalt]

The LWJGL team clearly says: Don't do thgat

[haughty copper]

in JPMS 😄

[glad cobalt]

Put one and only one on it

They also say that you should pre extract it

And not put it in there at all

[haughty copper]

Where are you finding that info 😅

[glad cobalt]

The first comment of the lwjgl guy on that issue you linked

His account seems at least very active

And he links to a bunch of documents regarding this

[haughty copper]

He's only commenting on JPMS

Oh you mean the general comments around pre-extracting it

[glad cobalt]

Yeah

[haughty copper]

yes, understandable. But it still is a supported configuration to have them all on the CP, while it's not for JPMS

Which kinda puts us in a bad spot

I mean, what's one more hack

Their jar manifests have an attribute I'll handle specifically. LWJGL-Platform: macos/x64

I am pre-indexing manifests in the startup code and caching the results so this will be very fast

And I think filtering out unneeded natives this way is better than trying pathname patterns..

[glad cobalt]

Yes 100%

Although we should report this to Mojang

Since the lwjgl guy is right

Mojang should not be depending on this mechanism in production

[haughty copper]

Hey but real-talk

They have millions of users

If this didn't work

They'd probably know that better than the LWJGL guy 😄

I'd almost bet they are the heaviest LWJGL users

I'll still put one stone into the "why I don't like us using modules bucket" 😛
And grumpingly fix it anyway

"grumpingly".... is that even a word

[haughty copper]

Alright:

[162] INF Skipping C:\Gradle Home\caches\modules-2\files-2.1\org.lwjgl\lwjgl-freetype\3.3.3\82028265a0a2ff33523ca75137ada7dc176e5210\lwjgl-freetype-3.3.3-natives-windows-arm64.jar due to incompatible architecture
[162] INF Skipping C:\Gradle Home\caches\modules-2\files-2.1\org.lwjgl\lwjgl-freetype\3.3.3\15a8c1de7f51d07a92eae7ce1222557073a0c0c3\lwjgl-freetype-3.3.3-natives-windows-x86.jar due to incompatible architecture
[162] INF Skipping C:\Gradle Home\caches\modules-2\files-2.1\org.lwjgl\lwjgl-glfw\3.3.3\f27018dc74f6289574502b46cce55d52817554e2\lwjgl-glfw-3.3.3-natives-windows-arm64.jar due to incompatible architecture
[162] INF Skipping C:\Gradle Home\caches\modules-2\files-2.1\org.lwjgl\lwjgl-glfw\3.3.3\32334f3fd5270a59bad9939a93115acb6de36dcf\lwjgl-glfw-3.3.3-natives-windows-x86.jar due to incompatible architecture
[162] INF Skipping C:\Gradle Home\caches\modules-2\files-2.1\org.lwjgl\lwjgl-jemalloc\3.3.3\ba1f3fed0ee4be0217eaa41c5bbfb4b9b1383c33\lwjgl-jemalloc-3.3.3-natives-windows-arm64.jar due to incompatible architecture
[162] INF Skipping C:\Gradle Home\caches\modules-2\files-2.1\org.lwjgl\lwjgl-jemalloc\3.3.3\f6063b6e0f23be483c5c88d84ce51b39dc69126c\lwjgl-jemalloc-3.3.3-natives-windows-x86.jar due to incompatible architecture

[glad cobalt]

Very nice

Yeah that makes the most sense

I personally feel that is also a better solution, regardless of CP or MP

There should simply be only one

And not many

[haughty copper]

Using the same JPMS name for different artifacts does not seem like a good practice

[glad cobalt]

?

Ah yeah

It is n't

But also in this case it should not matter

Like why would you put the different modules on the CP if the architecture of the system is nknown

So having JPMS block you from doing that

Seems reasonable

[haughty copper]

I bet almost no one uses LWJGL modular

You noted that the dude said to pre extract the natives, right?

[glad cobalt]

That was though in a general context

Unrelated to JPMS

[haughty copper]

They have hard deps from their platform independent JPMS modules on the native one

So you still have to include a native JPMS module on module path even if you had pre extracted

[glad cobalt]

Yeah true

But again

I think having more then one instance of a different none-compatible arch on the cp is not a great idea

It likely won't hurt

Since it will do a scan

And filter out

But you still can run into issues

[haughty copper]

It doesn't scan and filter it just does getResource for the file it needs

It works perfectly fine

JNA does the same and just ships all arch's in its main file

His point about extraction of DLLs to temp dirs is correct however. But everyone does it

Note that the different arch jars do not have overlapping resource paths. They are all distinct and unique

[glad cobalt]

Still

Yeah I understand that it is then unique at runtime

But still feels wonky

From an architectural perspective

[haughty copper]

It's platform independent

So I don't see the problem hehe

I bet their.natives don't even unpack.when used with jlink heh

[glacial crag]

Real talk: vanilla doesn't run with everything on the module path, right?

I.e. if you just -p everything

[eternal wren]

Vanilla doesn't even use the module path

[glacial crag]

Of course it doesn't. But I'm saying that it's not even a vaguely supported use case. It straight up doesn't work.

[eternal wren]

Nope

[glacial crag]

So remind me why we are wasting our time with modules again? 😁

When clearly Minecraft itself doesn't care

[eternal wren]

TBH modules provide a lot more benefits to the architecture

Especially if eventually you give us module-infos

[glacial crag]

Most of the Java community doesn't care about these supposed benefits. Which places us at odds with Gradle, IntelliJ, Minecraft, etc

The architectural benefits are largely outweighed by the downsides of having to work around most of the ecosystem

[eternal wren]

Most of the Java community still bothers supporting Java 8

With that logic, we should use that version

And TBH, IJ supports modules just fine; so does Gradle

[glacial crag]

Gradle and IntelliJ have full support for Java 21 yet shit support for modules

[eternal wren]

All applications I've been writing in Java ever since Java 9 came out were modular

And using IntelliJ and Gradle

I have to say, I have not experienced any issues whatsoever

Except for when I had to work around the system for an --add-exports

[glacial crag]

That's cool. If you write a simple modular app it will work. But anything nontrivial requires unofficial Gradle plugins and/or copious amounts of hacks

[eternal wren]

Then maybe have you considered that the problem here isn't modules, but the vast amount of non-standard stuff you gotta do to set up Minecraft?

[haughty copper]

We currently have 0

[glacial crag]

Modules are harder to set up than plain CP applications. We already have complexity coming in from Minecraft. We do not need additional self-imposed complexity from JPMS

[haughty copper]

I mean that is good for you, but doesn't really apply to modding a game that isn't modular

[eternal wren]

Then just remove modules

Since everyone is so against it

[haughty copper]

Oh I'd love to 😄

It's not like you'd actually notice

[eternal wren]

Let's go back to jar hell

[haughty copper]

We are in jar hell

[eternal wren]

You aren't giving us module-info

Of course we are

[haughty copper]

You said "go back"

We haven't left yet

[eternal wren]

All the groundwork for FML as far as I remember was for us to eventually be able to use proper module-infos

[haughty copper]

There is no no groundwork in FML

[eternal wren]

Whatever holds the architecture

[haughty copper]

Also who is "us" in that sentiment. Modders? Or us, the platform?

[eternal wren]

both

As far as I remember, cpw had mentioned intentions of getting module-info working for mods

[haughty copper]

If you are suggesting we should try to convert every modder to use a modular setup even at compile time, I think you underestimate the complexity significantly

If an individual modder wants to go module-info, they lose the ability to use any mod-apis of mods that aren't using module-info

And for what? "Doing cool stuff"?

People are already failing even at the basics of buildscripts

[eternal wren]

It's clear I'm in the minority here

[haughty copper]

In droves

[eternal wren]

So there's no point in defending my views TBH

[haughty copper]

Just to be sure my point is not misunderstood

I'd love for this stuff to be viable

But when weighing it E2E, I don't see the benefits outweighing the downsides

And you know, I've already wasted countless hours on trying even though I've forseen that it's a pain in the ass 😛

[jovial vault]

IMO it would be nice to have compile time modularity, but I don't see the point in runtime modularity enforcement

mostly because of how annoying it would make it to try to make addons of a mod without addon API

[haughty copper]

Well yes, it would be nice

[glacial crag]

Do you have any clue what needs to go into the Gradle setup to have compile time modularity?

[haughty copper]

Well it's an entirely separate topic 😄

[glacial crag]

Also how will that addon even compile if there is compile time modularity

[haughty copper]

Well if you compile your mod modular, I don't think you can access another mod that isn't modular

You might, if they declared an Automatic-Module-Name and that actually matches their mod-id, but no guarantees

But you're already dead in the water since the generated minecraft artifact doesnt have a module descriptor

and whats even worse, it won't work at all since it's a merged jar

[glad cobalt]

We can relatively trivially create a module-info.java for minecraft

[haughty copper]

I mean if you completely special case it and then ignore the resulting mods module-info at runtime

sure

otherwise. how exactly are you planning to deal with runtime having two modules (minecraft + neoforge)

while compile-time only has one jar

[glad cobalt]

That is an architectural decision that we have right now. We could trivially make that two modules

You already made the biggest step in NFRT, where you only recompile patched MC sources.

So you could trivially, create a minecraft jar and a neoforge jar

And actually have two modules

[haughty copper]

If we didn't actually do modular runtime, then yes, wouldn't matter. You could still do that at compile-time to get the benefits of better checking there

[glad cobalt]

Modular runtime is of interest though

[haughty copper]

Not really though, anyone can trivially sidestep any checks anyway

[glad cobalt]

By that argument again we should be back to java 8

[haughty copper]

If it's just a honor system

[glad cobalt]

Then enforce it

[haughty copper]

You can also just do the compile-time checks

[glad cobalt]

Anything not exposed won't be transformed

And then there is nothing to sidestep

[haughty copper]

And all that for... what exactly?

We're modding a game and don't give a fuck about Mojang making things private

But mods are somehow precious?