FML Clean-up

[glacial crag]

It is an architectural decision that we can and should reconsider

[haughty copper]

It's not really architectural it's more a technicality

it's split in prod

[glad cobalt]

You mean the fact that we have one jar at compile time..... Probably. If we could figure out a good way for neodev, where cross combined compile is not in the way.

[haughty copper]

It can't be modular in neodev anyway

same source-set compile

[glad cobalt]

That is what I mean

If you can figure out how to split it

[haughty copper]

I am not even sure it can be in moddev for the recompile

[glad cobalt]

Then the entire stack becomes pretty trivial

[haughty copper]

you'd have to fake the module-info post recompile

[glacial crag]

Ah yeah. I mean that in userdev we should be able to split it

[glad cobalt]

Userdev is easy

You are already doing everything you need for that in NFRT

[glacial crag]

But this is only a detail compared to all the work that is required to support modules

[haughty copper]

the split yes, but you still need to generate module-info not using the standard approach

I can see benefits in the compile-time case

it actually does something for users

runtime is just lots of work for pretty much nothing

[glacial crag]

I wonder how hard it is to hack it together at compile time

[glad cobalt]

Except that when this was designed the normal CP way really was ugly AF, and people trivially fucked it up.

[haughty copper]

I am not sure either, it's a completely orthogonal problem

[haughty copper]

Sorry I don't buy that, really

I have been doing fabric for years

and have NEVER run into the problem of mis-loading classes

[glad cobalt]

I only have second hand knowledge

[glacial crag]

We can setup proper classloader isolation with a CP environment too

[haughty copper]

not once

During the same time frame

I can't even count how many times I have run into "module fml_loader reads more than one module fml_blahfasel"

in Forge 😄

[glad cobalt]

Maybe

[haughty copper]

We are also one step further

I still think we can use a JavaAgent as a dev-time safeguard

if we'd actually wanna go CP

since the JavaAgent can get global callbacks on classload

for ANY classloader

and validate with a nice stacktrace when someone is doing shit like Classloader.getSystemClassLoader().loadClass("net.minecraft")

which borders on malicious

(and would not actually do anything in Prod, mind you)

Since in prod, the deobfuscated Minecraft classes you'd not want to double-load aren't even on the system CP to begin with

So you couldn't double-load them even if you tried

BTW Orion the situation was slightly different when the bus still had to be on the game layer

Because that definitely sucks

But now it's only MC+NF and those aren't loaded via CP in prod

[plain solar]

People still break it because mixin plugins load on the game layer and can load an MC class too early

[glad cobalt]

Yep

[glacial crag]

That's a problem for both Fabric and NeoForge

Unfortunately we can't solve it I think

At least Java coremods are safe here 😛 (who'd have thought them safer than mixin plugins)

[plain solar]

safer in that way 100%

[glacial crag]

Yeah only in that very specific way ^^

[haughty copper]

Okay but that is not really class-loader related, rather that it circumvents the transformer setup, right?

[plain solar]

It prevents targeting that class with a mixin

[haughty copper]

I think the classic issue we've been talking about is that you load a MC class in dev in a parent CL of the transforming CL

[plain solar]

Ideally people should not be able to start loading game classes at all until after the transformers are fully established

[haughty copper]

That should actually be doable

Although we can probably only do it heuristically for the vanilla packages (net/minecraft, com/mojang)

do mixin plugins actually need access to the game classes?

or could these be loaded disjointed from the TCL?

[plain solar]

They shouldn't

[haughty copper]

But probably to other classes in the mod

[plain solar]

However many probably rely on accessing another class from the mod

yeah

I think the community will riot if we require mixin plugins to be in a service jar

[haughty copper]

I was about to suggest it 😄

[plain solar]

even if we allow service-inside-mod

which I think we do now

[haughty copper]

we do

you can do some split package magic too

similar to what mixin does

but eh

[plain solar]

Tbh this architecturally makes more sense so it might be worth pursuing anyway

but multiloader devs will have pain

[haughty copper]

probably short-circuiting in the TCL when you try to load net/minecraft before the setup is done

is easier

[plain solar]

when is setup done though

mixins are lazily initialized on the first attempt to load a class iirc

[haughty copper]

yeaaaaaaaaaaah

Why are you making such a good point embedded, can't you leave me in my state of ignorance for a little bit longer

[plain solar]

I try not to make bad points

ok so here's a practical concern with the service jar idea

where do you put the mixins

mixins by definition compile against the game classes

[glacial crag]

That is already the case

[plain solar]

it would make more sense to manually load mixin plugins on another CL

[glacial crag]

The problem is with how mixin operates

[glacial crag]

Yes, definitely

[haughty copper]

How much control does the mixin service have over this?

If any

I think we should pull that into our code-base, BTW.

FML already has a hard dep on mixin, that would be the right place for it

[eternal wren]

Uh, Mixins could freely live in the service JAR

They don't need access to the game after compilation, do they?

They're never loaded

Except for accessor mixins, of course

Exceptions everywhere

Fuck

[plain solar]

Yes but then you can't have proper compile-time isolation of the game layer

[glacial crag]

Mixin already prevents you from loading them so it doesn't matter

[glacial crag]

Very little I think

[haughty copper]

Tech where is the event bus started again

I am getting into bootstrap now, but fail at

java.lang.IllegalStateException: Attempted to post event of type RegisterCommandsEvent on a bus that was not started yet!

😄

I wonder what I am forgetting

Oh nevermind, I'll just breakpoint in a normal instance

Oh god I think in FML dev I am pulling in an ancient version of eventbus

Yuuuuuuuuup. 7.x.x

Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make static long org.lwjgl.system.MemoryManage$DebugAllocator.untrack(long) accessible: module org.lwjgl does not "opens org.lwjgl.system" to module minecraft
Now we're in the modular weeds

[plain solar]

they still use this hack?

[haughty copper]

I think its for debugging only

But anway, now we gotta play add-opens for Mojang

[glad cobalt]

Time to generate the module?

Or what?

[haughty copper]

No, time to manually record all the things that just work when it's all in the unnamed module and open it using instrumentation manually 😛

I think there are 2-3 things

[glacial crag]

So this little thing was always hidden by SJH making every module open?

[haughty copper]

yes

[glacial crag]

"module support"

Lovely

[haughty copper]

Don't be snarky 😛

But yes, the point needs to be made: now I am writing a custom system that replicates the --add-opens JVM CLI parameters

We're just lucky Mojang didn't rely on --add-opens for their stuff, SJH currently would not be able to handle it

Yay. First time main-menu

[glad cobalt]

Nice

This is without args?

[haughty copper]

Yeah

and no BSL, main method in FML

essentially using ML piece-wise as a lib

[glacial crag]

You're missing the vanilla resources I think

[glad cobalt]

Yep the client extra jar

[glad cobalt]

That is exactly the same architecture I have in my experimental design

I find that a much better approach

[haughty copper]

No the assets

I didn't pass the asset index

if the client resources were missing you'd not even have the button textures 😄

[glad cobalt]

Right

[haughty copper]

The stuff I needed in ML: https://github.com/McModLauncher/modlauncher/pull/128/files (so far)

[glad cobalt]

https://github.com/McModLauncher/modlauncher/pull/128/files#diff-91ae8901ec44c4f4e36a4e429faf1d7dde4e84012f969d1ed0d9a9869752a27cR60-R61 ?

Why are we still doing this?

[haughty copper]

It's incorrect, tbh. but I haven't hit the inveitable bug it causes yet 😄

[glad cobalt]

Okey

This looks pretty good

[haughty copper]

I tried to make constructors allow for dep-injection where possible since that mostly prevents having to make things public

it doesnt work in every spot

[glad cobalt]

Architecturally I don't like the .resourceroot approach, but I know that for right now we don't really have an alternative

[glad cobalt]

I saw

This is a first step

My ML clone has like this all majorly refactored

To the point

Where FML even handles its discovery

I hate that ML does its own discovery

But that would require another major refactor of locators

[haughty copper]

Yes, this is currently trying not to break API if it can avoid it

I wanna try this with ATM10 / Enigmatica 10

[glad cobalt]

Understandable

[haughty copper]

Otherwise I'd also have refactored some other spots. I.e. in one spot it is leaking Environment as a method return type

[glad cobalt]

I did not set my self that goal XD

[haughty copper]

where it could have used IEnvironment instead

making me have to use the concrete impl-class instead of just implementing a facade on top of the FML env

[glad cobalt]

I had a clean design in my mind

https://github.com/McModLauncher/modlauncher/pull/128/files#diff-91ae8901ec44c4f4e36a4e429faf1d7dde4e84012f969d1ed0d9a9869752a27cR127 here right?

[haughty copper]

Yup exactly 😄

And since that is public in Launcher

[glad cobalt]

Which was a private class

Oh no it was public

The constructor was private

Arg

[haughty copper]

The class was public, the ctor wasn't

yeah

So.... no dice 😄

[glad cobalt]

Shite

[haughty copper]

So far at least it seems feasible to get rid of LCP

I mean it's simple to summarize the idea

[glad cobalt]

Okey this is looking pretty good

I understand the idea

😄 I have been following this pretty closely

[haughty copper]

We whitelist a known set of modules that we know we need to run our boostrap code

That is pretty much it 😄

[glad cobalt]

Yeah

[haughty copper]

the rest runs through normal discovery hehehe

I have not yet tried out the in-dev mod grouping. My current idea is actually more closely aligned to what NG did initially where it associates mod-dev directories with a mod-id... I'll see when I get to it

[glad cobalt]

I always considered that the better approach personally

[haughty copper]

If we write that mod-id into a file that we can discover relative to the directories on the CP, we dont need to pass anything to FML to figure out that grouping when it starts, that's why I am considering it

[glad cobalt]

I am still not 100% happy with it though

[haughty copper]

It'd be "fool proof"

[glad cobalt]

But that is mostly related to the fact that IDEA, Eclipse and Gradle all have a different model

Right now I am considering making a specific model for each IDE

[haughty copper]

Because that always works regardless of how the directories end up on CP

[glad cobalt]

Instead of trying the "set it up once" and hope for the best appraoch

As an extension to IDEA / Eclipse models for example

[haughty copper]

We also do not need to know about the directory layout cross-project

It has downsides ofcourse: a common project still needs to write that mod-id to a file location, but that could be done without having to rely on a full-blown moddev plugin

[glad cobalt]

If it is simple enough: Could be a static file like MANIFEST.MF for most cases can be

[haughty copper]

no because you can't identify the classes dir because of it

I'd currently favor something under build/

and you scan essentially for build/modgrouping.propertiesor whatever

by walking upwards from each classpath directory you found during discovery

the only assumption that needs to hold for that to work

is that output directories must be beneath the project directory

but that is true for all IDEs and Gradle unless you manually chose to break that assumption

[glad cobalt]

Is that true for idea in module specific compile mode?

[haughty copper]

yeah

it becomes murky when you switch it to non module specific compile 😄

as in inherit output directory from parent project

still need to investigate that, but it's at least still within the gradle project structure

I mean, at some point people can always manually change some project setting to break everything

[glad cobalt]

Is that not the default thoigz

[haughty copper]

No the default is this:

Eclipse also by default compiles into bin/

[glad cobalt]

Okey

So maybe this could work

Make sure to write down the specifics

[haughty copper]

Yup, first i need the game to launch perfectly again though 😄 Gotta still test the Mixin aspects

[glad cobalt]

I was thinking about the whole mixin discussion

[haughty copper]

Alrighty, took me a biiiiiiit longer than I had hoped but it had a lot of wrinkles

I now have a buildSrc plugin in FML that is able to make standalone MC installations into build/ using the NeoForge installer

And it adds a sourceSet / sets up the classpath so it's possible to run that from IDE

Finally I should be able to test the client prod environment properly from IDE, heh

[haughty copper]

If anyone is interested, this is a minimal self-attach without bytebuddy or similar, which works for our target VMs: https://gist.github.com/shartte/c1b4e2becebd5e603189b52e2e6aeaad

[haughty copper]

Aight, adding support for '@'-files directly to my startup main method, that makes passing it the asset location far easier.
Alternatively we might support asset locations via classpath files 🤔 Similar to what coeh did in Neoform

[haughty copper]

Hm. Why is there no Module-Path Manifest Attribute equivalent to Class-Path?

I suppose we shall use the same hacks for the executable jar as we do for the client

[glacial crag]

there was a QR code in earlydisplay?

[haughty copper]

Yes.

It leds to a blogpost by maty that doesn't exist anymore 😄

[glacial crag]

😄

[haughty copper]

It leads to https://tinyurl.com/forgetest

[glacial crag]

ah yes

[jovial vault]

did it ever work as a QR code?

[glacial crag]

regarding https://github.com/neoforged/FancyModLoader/pull/177

a lot of it is just boilerplate that already exists in other Gradle plugins

[haughty copper]

I did scan it

[haughty copper]

Does it though 😛

As a public API

[glacial crag]

of course not

[haughty copper]

They also all do not run the installers

And run configurations are never offered in a generic enough way

[glacial crag]

yeah I suppose that the prod ones are not worth trying to abstract

how do the dev ones work?

[haughty copper]

It's just mdg

initially I had those use MDG artifacts from a second subproject

but I did change it to be just MDG now

[glacial crag]

oh of course lol

does it just pull in the local FML automagically?

[haughty copper]

i added a dependency further down, but yea

[glacial crag]

you can remove the disambiguation rules, probably

and we might want to move that to MDG's API

[glacial crag]

where is that?

[haughty copper]

https://github.com/neoforged/FancyModLoader/pull/177/files#diff-61505fa65b4a1c363b0d681ccbeb019c43fe20ebfc3630110cda8fefd633a3b4R43

"further down" -> wrong direction 😄

in the configureEach

Good point that it might actually be borked for moddev, good find lol

[glacial crag]

ok that's what I thought 😄

in any case, it will take some time to review

[stable wyvern]

we should replace it with a rickroll and re-enable it

[haughty copper]

@glacial crag fixed the moddev deps

[glacial crag]

first time I hear about UpdateDaemonJvm

seems like a new 8.8 feature

[haughty copper]

Yes it is

But as it turns out it also just selects what you have installed locally. I'd thought it really hooks into toolchains, but doesn't

[glacial crag]

    repositoriesMode = RepositoriesMode.FAIL_ON_PROJECT_REPOS
    rulesMode = RulesMode.FAIL_ON_PROJECT_RULES

why?

[haughty copper]

because footguns?

What do you expect happens when you have central repo management, and declare project-local repos?

It doesn't append, it fully overrides anything from settings.gradle. Those two settings are there to prevent that from happening accidentally.

[glacial crag]

oh ok

I didn't look up what that does 😅

[haughty copper]

Yes, essentially it throws if you try to declare repos in the subprojects

which would kill your centralized mgmt for that repo

[glacial crag]

hmmm

why is it not picking up the module, again

let's have a look with the debugger

grrr why is gradle not waiting for my debugger

Unknown command-line option '--no-daemon'.

WTF

what is intellij doing???

oh it's your deamon crap @haughty copper

I'll yeet it then

😛

it breaks remote debugging and --no-daemon

[haughty copper]

MY DAEMON CRAP ?!

And you do not have IJ launch enables, yes? because it does not work yet

[glacial crag]

no no

[haughty copper]

Are you trying to run this with your IJ from 1999 again?

[glacial crag]

this might be an intellij problem

2024.1.1

built on April 29, 2024, come on 😛

[haughty copper]

Since it's working here, I'd expect that

[glacial crag]

try running :test:build --no-daemon --no-configuration-cache -Dorg.gradle.debug=true from IntelliJ

[haughty copper]

why the hell are you running with nodaemon

[glacial crag]

why does that option not work? 😭

the debugging isn't working either

[haughty copper]

It hasn'T done anything useful in forever

I seriously have no idea what you are doing, I've been right-click debugging the gradle tasks in that project all the time 😅

[glacial crag]

I want to debug MDG

I could also use an included build, but in theory remote debugging should work too

-Dorg.gradle.debug=true should be enough for remote debugging

[haughty copper]

No idea, tbh. I've only done right-click debugs

and yes, includeBuild is way easier

First attempts at loading ATM10 with the experimental startup code 😄

I haven't pulled tags, so my local FML is version 3.0.x 😄

What does this mean?

! [rejected] 4.0 -> 4.0 (would clobber existing tag)

[glacial crag]

you have a wrong local tag?

[glacial crag]

classic! 😄

[haughty copper]

Maybe? I don't remember that, tbh 🤔
But okay --force it is

[glacial crag]

MDG computes a module name of FancyModLoader.test.main

it should be FancyModLoader.test

...

that's my fault I think

[haughty copper]

no?

the project name is test

FancyModLoader.test.main sounds right

Soaryn riding the bleeding edge wave

[glacial crag]

wait

WTF

[haughty copper]

well yes

I have noticed that too

IJ is going CRAZY

even without the minecraft addon

[glacial crag]

I guess that test is ambiguous

[haughty copper]

this is exactly the bug we get, when the minecraft addon is installed

and destroys your runs

but it's not ambiguous....

[plain solar]

I just fix them every time they're generated at this point

I gave up trying to diagnose it

[haughty copper]

But honestly this is the first time I had it without the plugin

[glacial crag]

FancyModLoader.test could refer to the test subproject, or to the test sourceset of the main project

[haughty copper]

so I am making the assumption it's definitely some quirk in IJ

[haughty copper]

Ooooooooooh 🤔

[glacial crag]

I bet that it has a check for that exact case, and implements this workaround

[haughty copper]

😄

And now I have to cash in my debt of not correctly emulating ModLauncher service discovery in FML

[glacial crag]

wow, you can abuse getInputs to make some properties lazy

that's what you're using it for!

[haughty copper]

Yep

[glacial crag]

that's crazy 😄

[haughty copper]

Without subclassing

And just a static doFirst

Luke said I could have used a lambda and captured the providers instead, but I find it too easy to capture the project or other stuff that will blow up in our faces

[glacial crag]

ah yeah you have a doFirst like that

I thought it was just overriding one of the properties in the task using magic

not as fancy as I thought

[haughty copper]

Heheheh okay yes I can see how that would suck

okay, what do we name the subproject

gametest?

[glacial crag]

tests?

is the whole subproject new in the PR

hmm yes

[haughty copper]

integrationtests

?

[glacial crag]

I think that it's too easy to confuse with MC gametests

[haughty copper]

Yeah iti s

tests is fine

[glacial crag]

👍

I can do it if you're working on your other branch

[haughty copper]

I already switched, so I can do it

I need it on the startup-experiments branch too, so I am eager to do it. I also run into the module problem there

[glacial crag]

// TODO: Use MDG utility since idea-ext is just wrong

this only matters for projects with a space in their name, so we should be fine with the ModuleRef

[haughty copper]

Done & pushed

[glacial crag]

👍

[haughty copper]

Yeah I kept the TODO we ever would want this to go into MDG

[glacial crag]

this is resolving a config - bad

[haughty copper]

I really don't want to until we adopt the new startup code that makes us no longer need module path and LCP

[haughty copper]

It should only do that in the IJ sync though 🤔

There's no prepare run task I can attach it to

I'd just leave that since it'll be gone with the startup experiments and still works fine with config cache

[glacial crag]

all of the idea-ext logic runs even without the sync

[haughty copper]

Hm true

[glacial crag]

I remember being surprised by that some time ago heh

[haughty copper]

No it's right, I shouldn't be. But that's what we had idea.sync.active for in MDG

But the config should be minimal so eh...

[glacial crag]

yeah it's ok

[haughty copper]

Also: this doesn't leak to modders and is purely local to FML development

So I am willing to be a bit more lenient there

[glacial crag]

it's interesting that RunConfigurationSettings is not using the same naming scheme as RunModel for its methods

[glacial crag]

yes same

we could even reuse RunConfig but it would be hacky

[haughty copper]

Hmmm, I can correct that I suppose

Yes, I am rather thinking the opposite, we might be reusing the generalized run configuration plugin at some pont

*point

[glacial crag]

you should also copy the docs over to MDG 😄

[haughty copper]

if we also make it eclipse capable that is

[glacial crag]

yeah

[glacial crag]

we might want to add a comment here just in case we decide to reuse the plugin and somehow still have a module path

[haughty copper]

the game directory vs. working directory difference is actually deliberate, btw

since the run configs are generic

[haughty copper]

@glacial crag I changed the prop names to be more MDG like for program and JVM args

[glacial crag]

YAGNI!!!

but ok 😛

[haughty copper]

Well gameDirectory is also a bit of a shit name

[glacial crag]

is that also why you added a property for the task group?

[haughty copper]

because we tie it to the same directory in FML

but it's not actually necessarily true

that the working directory == the gameDir

[glacial crag]

well that is true

[haughty copper]

And also it's possible we can use this for non-game runs

so yes yes, YAGNI and all but workingDirectory isn't a bad name 😛

and yes, same for task group

[chrome mortar]

the fuck does YAGNI mean

[glacial crag]

you ain't gonna need it

the overengineering killer 😛

[chrome mortar]

can't even pronounce that acronym lol

[glacial crag]

yaghni

again!

should we do the post sync task hack?

[chrome mortar]

to create the working dirs?

[glacial crag]

yes

we can also do it with a valuesource

[chrome mortar]

I mean, I feel like you can create it at project eval time lol

remember this won't ever touch "production" code

[glacial crag]

I'm aware but it's a bad practice nonetheless

[chrome mortar]

it's purely for testing, you can skip some "best practices"

esp if they increase the amount of code 😛

[haughty copper]

No I think the production installs take too long for that

the working dir along doesnt help you

you need to run the install task

[chrome mortar]

wasn't KISS your favourite acronym after all

[glacial crag]

ah

[haughty copper]

I actually think our installer takes long.... and might take a look at it, at some point. But currently, just run the two install production tasks to get it set up

[glacial crag]

so the run is not even working out of the box?

[chrome mortar]

well, you can blame fart for paprt of it

and bin patches taking ages to decompress for the other part

[glacial crag]

yes; but hacks are not necessarily simple 😛

[chrome mortar]

I'm very certain that fart logging so much shit is slowing it down considerably

[haughty copper]

Nope you need to install

[haughty copper]

Yes, same thought

It's 80% of the logfile

Binpatches might be less of a problem once there's 60% less of them

heh

[glacial crag]

Caused by: java.util.NoSuchElementException: No value present
    at java.base/java.util.Optional.orElseThrow(Optional.java:377)
    at MC-BOOTSTRAP/main/net.neoforged.fml.loading.targets.CommonLaunchHandler.overwriteLoggingConfiguration(CommonLaunchHandler.java:68)
    at MC-BOOTSTRAP/main/net.neoforged.fml.loading.targets.CommonLaunchHandler.preLaunch(CommonLaunchHandler.java:57)
    at MC-BOOTSTRAP/main/net.neoforged.fml.loading.targets.CommonLaunchHandler.launchService(CommonLaunchHandler.java:116)
    at MC-BOOTSTRAP/[email protected]/cpw.mods.modlauncher.LaunchServiceHandlerDecorator.launch(LaunchServiceHandlerDecorator.java:30)
    ... 11 more

when running the prod client 😄

it didn't find the fml_loader module

HUH

ah findModule will also look through parent layers I guess

yeah FML is actually loaded as the main module 😅

yet another problem with modules...

they just don't match the IDE model :/

[haughty copper]

Hm, okay but you are running through IDE, yes?

As I said, run the gradle tasks for now, that's an aspect that is fixed by my new startup code, even for module mode

So whats your general take Tech?

[glacial crag]

hmmm okay

but then why do we have the run plugin if the runs don't work 😄

we could add gradle tasks to intellij instead

[glacial crag]

the run plugin is fine; I haven't looked into the installer

my overall impression is that this dance is just stupid and FML should be moved back into NF 😛

buuuut for now we'll just have this in FML; it's better than nothing

I'll continue looking at the other files tomorrow

[haughty copper]

Well yes, but you'd still need to run the installer to get a non-neo-dev environment for testing

OTOH you would get a neodev environment for testing, heh. since that I can't easily emulate

Maybe I have to add half of neodev to NFRT to actually do that blergh

[glacial crag]

well I mean that the dev tasks would at least make more sense if we had FML and NF in the same repo

the prod ones are a different story yes; in principle we could just build the installer then run it

[haughty copper]

hm yes, but the tasks would largely be the same

and you'd still have to have the run config plugin

so you end up with exactly the same amount of gradle soup

ultimately the only difference would be that you shove the task output of neodev into the installer configuration

[glacial crag]

yeah that is true

[haughty copper]

And the IJ run configs should btw work if you run through Gradle

[glacial crag]

they don't

[haughty copper]

Hmmmmm

Oh right

[glacial crag]

Gradle won't put the jar on the CP

[haughty copper]

They don't actually run through gradle

it's some special IJ thing

[glacial crag]

yeah you are right actually

it only builds with Gradle

but even a Gradle run would usually not run with the full jar, would it?

[haughty copper]

But yeah, I left them in since I can use them in the startup branch. BSL just doesn't support grouped module folders at all

it would

gradle runs with the actual jars

and it works

[glacial crag]

yeah, runProductionClient works

[glacial crag]

I find that suprising actually

hmmm no it makes perfect sense for classpath project(":loader")

ok

I'll have a deeper look at the rest tomorrow; it's sleeping time!

[haughty copper]

👍

[haughty copper]

So I got ATM 10 to launch with the new starter but ahem without promoting the CP to modules at all

While still using the layers beneath

[glad cobalt]

Do you still need the ModLauncher PR?

You closed it?

While I was reviewing it

[haughty copper]

Wait

Which one

Ooooooooooops

That was unintentional, I think I was trying to clean up some branches

But in any case @glad cobalt So what is interesting for productio is this scenario:

• There are no classes on the app cp that shouldn't be loaded there anyway
• setting the parent CL for the first layers modular CP kinda "just works" without any JDK internals used
• if I keep my "modular hoisting" in place, runtime fails because some mods access internals of i.e. gson and are then rejected by the standard JPMS access checks

At least ATM fails to load because kubejs accesses gson.internal, and loading gson into a JDK loader enforces the access check, while it previously did not due to SecureJar making everything open

[glad cobalt]

I would really like those checks

But it is for sure a breaking change

I am very much a proponent of them

Because they are just part of the JPMS system

And they should have been there in the first place

In the same way we should really be generaing a module info for minecraft / forge

[glacial crag]

We shouldn't stop modders from reflecting into MC

I don't really have an issue with modders reflecting into the internals of gson myself heh

I'm surprised that gson even has a module-info heh

[glad cobalt]

No for sure, our module info should just expose every package

We can trivially generate that at compile time though

[glacial crag]

That or just make the module automatic

[glad cobalt]

Hmm

Is that not what he currently has?

[glacial crag]

That's what we do at runtime now yes

[glad cobalt]

Yeah, but I am not arguing about now

[glacial crag]

But it won't work for libraries such as GSON unless we force-open everything

Which we should easily be able to do with instrumentation if we wanted to

idk. Using modules then making everything open sort of defeats the point anyway

[glad cobalt]

Not really, I mean for minecraft it makes sense

But for GSON, which explicitly says: don't access my internals

Or from other mods or libraries

Which explicitly have this

It makes all the sense in the world to me

It might even make sense for us, to have our own internal package

That is not reflectable, or accessible this way

Which helps with the whole compatibility between versions thing IMHO

[glacial crag]

Why would we let gson dictate our choices when we don't let MC dictate our choices

[glad cobalt]

MC does not have an internal definition of what should be public and what should not

So we can reasonably make up our mind

[glacial crag]

IMO modders using internals are responsible for keeping that part of their mod up to date

[glad cobalt]

And in our case it makes sense to make everything public no?

[glacial crag]

MC has private members that we regularly make public

[glad cobalt]

Yes so?

What is the point here?

Again we explicitly make the decision to make this public

[glacial crag]

It has a definition of what should be public and what should not, in a loose sense

[glad cobalt]

The whole point of JPMS is that as module author, you get some control over the usage of your code

At runtime

By others

[glacial crag]

Yes, and I am saying that this might not be appropriate for a modding environment

[glad cobalt]

In your opinion

I would argue otherwise, looking at other modding APIs in other games, they all have some form of seperation of what you can touch, and what you can't

[glacial crag]

Most modding APIs are a lot more limited though

[glad cobalt]

So?

What is the point?

[glacial crag]

The point is that Minecraft modding is a lot more flexible, and that flexibility comes from being able to change literally anything

[glad cobalt]

Understandable, but there have been many modders, me included, that have long said, that this flexibility is always a pain to deal with. And that we should create some sort of restriction

At least when it comes to our own stuff

Aka, my mod, my code, you not touchy, to summerize

[glacial crag]

Unfortunately, such a restriction is ultimately limiting the freedom of other modders. I think it's problematic for the platform to encourage that

[glad cobalt]

So you are now limiting modders that do not want that?

What is that for an argument?

[glacial crag]

I don't limit you, I prevent you from limiting others

Subtle difference 😉

[glad cobalt]

No you do, because you limit me in my ability to govern how my code runs

By forcing my accessibility to be explicitly open

By making a change to the bytecode that I published

[glacial crag]

I don't change your bytecode, but I let others change yours

[glad cobalt]

No you do, you modify the bytecode I exposed in my module info

Because you force it to be open

Or you ignore it

Which ever way you want to look at it

[glacial crag]

Just like many folks rely on changing Minecraft and sometimes (Neo)Forge's bytecode

[glad cobalt]

I would argue the amount of people that want this limiting factor, to be able to clearly define API, and none API, vs the amount of people that need to hack into other mods, are about equal

[glacial crag]

There already exist ways to define a clear API (API jar, internal package, etc)

For example, Fabric has been doing it forever

[glad cobalt]

None of that really matters though

Because people can and do completely ignore it

[glacial crag]

It works very well on Fabric

[glad cobalt]

No it does not

I can trivially access internals

Modify what ever i want

Reflect into it

Mess with its bytecode

[glacial crag]

Yes, you can

[glad cobalt]

And as I said: My code, My choice

Not My Code, your choice

[glacial crag]

I disagree with that mentality. There is no need to be so controlling over your own code, imo

Lex might have encouraged a lot of that, btw

[glad cobalt]

Accept in the past I needed to be

because people hacked into my code on C&B to "fix" something

Instead they bricked the entire renderer

Or to make it compatible with Optifine

Which then broke all kinds of crap

And I had to spend ours figuring this out

And I am not the only one that has to do this

[glacial crag]

Hmmm yes ok that really sucks actually

I always forget how stupid people are when it comes to rendering heh

[glad cobalt]

They really really really are

And I am not saying that people like embeddedt don't do it ´the right way

But sadly experts like that

Are not the norm

The norm are idiots

Which don't provide the needed support themselves

Which don't have the know how

Or forget that maybe the mod runs on multiple platforms and as such does not behave the same as any forge or fabric mod on its own

And I get to spend hours and hours testing this

And it is not just frustrating me

But also my players

That download a pack

And then have to spend hours fixing it

And sure you could use this method to fix a real bug

But then again, most mods these days are open source

Make a damn PR

And if they are not, then you are an ARR land anyway

So you are not even legally allowed to do this, or are in a grey area at best

On top of that

Out of personal preference

I spend hours making large and very flexible APIs

That allows you to basically modify anything in anyway you like

[glacial crag]

Is the problem mostly mixins, or is reflection a problem too?

[glad cobalt]

Mostly mixins, but I have most fields defined in such a way that it can be done with both

I have only seen mixins though in practice

[glacial crag]

The two have vastly different solutions

And most importantly disallowing mixins is unrelated to modules

[glad cobalt]

Yes and no, as a personal opinion, and I said this to shartte before, is that we should only transform what is exposed via module info, and respect the module info

Because the module info defines what is publicly consumeable

And I see no reason why publicly consumeable stuff should not be transformable

By everyone

At the same time

That also limits reflection

[glacial crag]

Well it might still have non-public internals

[glad cobalt]

Because the JVM restricts that data niocely

[glacial crag]

If the module is not open you can never reflect by default

I don't think that exports allows deep reflection

[glad cobalt]

I think it does not, but I never tested it either

I think you can reflect a field in a type in a exported package, even if the type is not in an exported package

But I don't think you can reflect then into the fields and methods of the value of said field

Because that would be reflecting into a type that is not exported

[glacial crag]

You can't call setAccessible on a member that is not open I think

Regardless of the exports

[glad cobalt]

on a member with an exported type?

Or on a member in a not exported type?

[glacial crag]

On any non-public member

Regardless of its type

I think you can still find classes that are not exported?

But you can't do anything with them

No access at compile time, and no access at runtime

[glad cobalt]

Seems fine to me

[haughty copper]

What is the actual resolution here? We are currently not restricting anything whether it is our internals or the internals of libraries on the CP

I see no mandate to introduce any such restrictions at the moment

[glad cobalt]

What do you mean with no mandate?
I mean by default (if a mod does not provide a module info) we can still open it to all

But if a mod or library provides a module info

Then I think it makes clear as to what it considers accessible by others and what not

[haughty copper]

That is a change in architecture

[glad cobalt]

And we should respect that

Yes for sure

I am not arguing other wise

[haughty copper]

And no mandate means: we have made no decision to do that and have no consensus to do it, either 😄

[jovial vault]

fwiw, I have said this in the past, IMO it's wrong to restrict people's ability to modify other mods. it goes against what I believe is the core pillar of modding, which is to give the player the ultimate choice of how the game works, and if we say "this mod doesn't want to be modified, so you can't customize it", this pillar crumbles

[haughty copper]

I'd tend to agree, yes

[glad cobalt]

Except that this also has a different side: The player wants a modpack to actually "work", and there are now modders, not players, abusing that freedom, to cause problems for players

We have made restrictions as to what modders can do when in the past

To achieve the same balance

[haughty copper]

Did we? I thought generally we've just added more validation to catch bugs earlier

They still have the trifecta: JNA, Unsafe, ASM 😄

[glad cobalt]

ASM would not work in this case

Not sure how the JVM protects against abuse of JNA and Unsafe, but at least the latter will get removed soon

[haughty copper]

It doesn't since it cannot

We have multiple ways to access native memory

even if you ignore Unsafe

[jovial vault]

java does "the best it can" to remain sane when code uses native access

[glad cobalt]

It does

And IMHO we should follow that idea

We should be sane when we allow transformation

And if a modder, explicitly says: "Hey listen I don't think modifications to this area of my code is a great idea" then we should follow that

[haughty copper]

So anyway, it's pointless to litigate our differences in opinion about allowing mods to close themselves

So, how do we proceed? And what is the scope.

[glad cobalt]

IMHO I would like to see the module info to be respected

If a modder provides it, then follow it

If a modder does not, then open it to all

And I think we should generate an open all for minecraft and neoforge

[glacial crag]

Can we bring more maintainers into this discussion

[haughty copper]

Well yes, I understand your opinion, I just don't share it

How do we get to a decision

[glad cobalt]

We talk about it

[haughty copper]

Since the trade-off is that for a very niche use-case we drag along a massive technical investment

[glacial crag]

Let's bring it up in #project-talk

[glad cobalt]

This should be in #maintenance-talk IMHO

[glacial crag]

Just make a brief summary of the pros and cons before

[glad cobalt]

But we can do either one

[haughty copper]

Yes, it's not really #project-talk

[glacial crag]

Is it not? It has large implications I would say

[glad cobalt]

Maybe. I think it won't be used by many.

Just by those that are effected by those mixins at least somewhat reasonably often

[haughty copper]

Wait, opting out of transforms is a different story, and would not require use of module metadata. Actually it is orthogonal.

It's just whether you're open to reflection at runtime that's more or less the question of module metadata

[glad cobalt]

Sure, but as I already said above, I think module metadata describes in a proper JDK defined way what a modder considers to be public code.

As such it is a build in way that allows us to easily define what should be accessible by others, and as such transformable (at least I see no problem in accessible == transformable), versus what is not accessible to the public and as such should also not be transformable.

[haughty copper]

BTW if you publish such an artifact, it will have no effect at dev-time for consumers of your api

[glad cobalt]

I know that right now it is not used properly, but that is a different step altogether IMHO.

And possibly a parallel discussion to have

[haughty copper]

And you'd also like to then respect the module-metadata of the libraries?

Meaning KubeJS has to stop using the gson internals?

[glad cobalt]

Yes, maybe, should potentially be discussed that for libraries it can and maybe should default to fully open. Not sure.

I don't use internals of libraries. So I have no idea how far it would reach

[haughty copper]

Well what is it 😄

Because without hacking our own module loaders in again

[glad cobalt]

IMHO, if it where up to me, then yeah

[haughty copper]

we cant actually make them open

[glad cobalt]

If a module specifies: this is accessible, and this is not.

Then we should respect that

So yes

If that means GSON says, don't touch this

Then yeah a modder should not be able to do so

[glacial crag]

so program args can be added as argfiles?

provided that there is no explicit main class given to java I suppose (i.e. it also comes via an argfile)

[haughty copper]

wdym in which version?

I am adding this to my startup experiment, yes

[glacial crag]

I was checking the prod installations

[haughty copper]

ah!

yes, well, it remains an extremely useful feature to have

argfiles

[glacial crag]

Of course

But I'm surprised that you have a separate file for the main class

[haughty copper]

otherwise you could not override that individually

[glacial crag]

I don't understand

how is this easier to override than having a single file?

[haughty copper]

... by simply not including the main class arg-file

[glacial crag]

I don't understand

you mean the user can just pass something else as the main class to the run configuration settings?

[haughty copper]

yes

check the startup-experiments branch briefly

just in GH web-view

and check tests/build.gradle there

[glacial crag]

ah yes

but that's only required because it's using a neoforge that itself uses an old FML/BSL/etc

fine by me but that goes to show how weird the setup is

[haughty copper]

Yes, that much is true. I think this can still be simplified later but at the moment it is convenient for various experiments

[glacial crag]

other question: why are we hardcoding the daemon version in a task that is just writing a trivial property?

this updateDaemonJvm task seems incredibly silly

can you even run it if you are running on the wrong jvm version?

[haughty copper]

yeah we can rip that out

[glacial crag]

ok I left a number of comments

once they are addressed we are good I think

[glacial crag]

so I've been thinking a bit

we want dev and prod to match as much as possible

yet in prod we have a fixed classpath + a mods folder, while in dev we have a flexible classpath

in dev, we can already know the fixed classpath of the prod instance

so the first thing we should do in dev is split the classpath in two parts:

• the things that will be on the CP in prod (basically this means MC's own libraries and our core startup code)
• everything else

then we could treat "everything else" as if it were a mods folder? i.e. any library there would automatically get loaded into the "PLUGIN layer" by default, etc

[haughty copper]

i mean that is more or less what my FML startup code does using a hardcoded module whitelist

in the startup-experiments branch

i let everything else pass through mod discovery

and added a low-priority "make the rest a library" locator

i did not add all of vanillas libraries to the whitelist, since that is not necessary, really

And yeah this loads ATM10

[glacial crag]

couldn't some services want to make use of them?

[glacial crag]

I'm not surprised; it's the only way to go back to a single classpath I think

[haughty copper]

Yes, that is more or less a platform we also define as a subset of vanilas platform

[haughty copper]

correct, it is

well not entirely, if you went with non-modular, you'd invert this

and filter on the TCL

[glacial crag]

well I was reasoning with a non-modular setup

[haughty copper]

by applying a filter to load every jar you identified as participating in transformation there

if you're non-modular you do not need to do that

you can just do full discovery

when you identified everything that you want on the game layer

[glacial crag]

I'd actually build a new classloader that can only access our "whitelist" as the first step of init

[haughty copper]

you just ensure the TCL loads that locally

in non-modular

you should not do that

because it costs you time needlessly

you just use the app classloader as it is

[glacial crag]

what is preventing services etc from loading MC classes?

[haughty copper]

in production, there are no MC classes on the CP

except for the three entrypoints

[glacial crag]

that is true

[haughty copper]

in dev, we can validate this using the agent

[glacial crag]

isn't that slower than scanning the CP once?

[haughty copper]

in two ways, really. post factum and while it's happening

you're not just scanning the CP once, you're having java reload all the jars twice

the agent can give you a list of ALL loaded classes across ALL loaders

that can be used to just do a sanity check before you launch the game that anything you want on the TCL hasn't already been loaded in the top layer

the other option (we may just hide this behind a debug flag btw)

[glacial crag]

because URLClassLoader will keep a jar open, you mean?

[haughty copper]

would be to register a global transformer that makes sure the minecraft classses are not loaded on anything but a TCL

[haughty copper]

it has to reopen it if you construct a new URLCL

supposedly it is lazy

but if you do a classpath scan

you have obviously made the jars on the app cl nonlazy 😄

[glacial crag]

well can't you just scan java.class.path

[haughty copper]

okay you gotta be more precise when you say classpath scan .D

[glacial crag]

ok well I guess it's not trivial how you determine which jars are whitelisted and which are not

[haughty copper]

But generally I'd step away from that because other loaders don't need that step and we can have additional safeguards they currently don't use

[glacial crag]

fabric does exactly that

[haughty copper]

wait, seriously? 🤔

[glacial crag]

let me check

[haughty copper]

I mean in that scenario you're fucked with the FS providers again

So my general philosophy is: we don't need to take extra care in dev, in my opinion

since there's only three transform candidates on the system CP

[glacial crag]

https://github.com/FabricMC/fabric-loader/blob/d9a000670180e73569a6983c423dd4299278afda/src/main/java/net/fabricmc/loader/impl/game/LibClassifier.java#L100

isn't this basically loading one class from each dependency

[haughty copper]

Where is it constructing a new CL from that

[glacial crag]

https://github.com/FabricMC/fabric-loader/blob/master/minecraft/src/main/java/net/fabricmc/loader/impl/game/minecraft/MinecraftGameProvider.java#L239

which ends up here: https://github.com/FabricMC/fabric-loader/blob/master/src/main/java/net/fabricmc/loader/impl/launch/knot/KnotClassDelegate.java#L167

[haughty copper]

Yes but that is just used by the transforming CL to filter which classes to let pass through to the parent

at least that is my understanding of what it does

we can do that based on packages if we continue to enforce no package overlaps and scan packages per mod jar, which I'd perefer, honestly

[glacial crag]

well yes

that's what I mean by scanning the CP

I mean "find the jars that we care about" and "ignore the others"

but I suppose you'd have to scan the entire CP for mod discovery anyway

scanning for packages isn't free btw

[haughty copper]

you made it sound like you wanted to construct another URLClassLoader directly beneath the ClassLoader.systemClassLoader

with just the whitelisted URLs of the system classpath

[haughty copper]

It's trivially cachable

[glacial crag]

that's exactly what I wanted to do yeah

[haughty copper]

that's not what fabric is doing, and we don't have to do it either 😄

it just leads to fucking annoying problems

[glacial crag]

well how else do you think it achieves "classpath isolation"?

[haughty copper]

by doing child-first for everything except a subset of system libs in its TCL?

[glad cobalt]

Are you guys discussing removing modules?

Or where is this going?

[haughty copper]

I think this is a discussion on how that would even look like

[glacial crag]

how would it know whether it should do child-first or not for a given class name

[haughty copper]

in a hypothetical case for us?

[glacial crag]

I am talking about FLoader

[haughty copper]

Well they are putting no restriction on packages

[glacial crag]

I am 100% sure that it builds a CL with a subset of the URLs

[haughty copper]

and doing no package scanning either

so for them this is a viable option

[haughty copper]

well the TCL with the transformable classes

but that is the inverse of the whitelist 😛

[glad cobalt]

Is that not the whole idea of why we use modules?

We can technically assign a given set of modules a given classloader?

[haughty copper]

eh, well yes it maintains a package-name -> module map internally

[glad cobalt]

And as such a package -> classloader map

[haughty copper]

It's just one classloader though that matters, really

Ultimately it's "local modules" or up

[glad cobalt]

Yeah the TCL no?

[haughty copper]

yeah

you have to either decide to load the class with transforms or not, basically

[glad cobalt]

Which is IMHO the most important one for us

Correct

[haughty copper]

okay if we hypothetically would drop modules

we'd still have to scan every jar

for annotations at the very least

which is why I would not actually drop the discovery of packages per jar file

[glad cobalt]

I think that logical link between jar -> module -> classloader is the thing that it gives us

[haughty copper]

and still maintain the constraint that packages in jars are not allowed to overlap

[glad cobalt]

The problem is determining in what layer a module goes right

[haughty copper]

no we do that in the discovery phase anyway based on jar metadata before modules are even loaded

when you look at the output of our discovery it spits out a list of jar files that are supposed to go on the "game layer"

which corresponds to the transforming CL

if we have scanned previousuly which packages these JARs contain

[glad cobalt]

That sounds like reimplementing about Half of jpms

[haughty copper]

you already have the info you need to filter every loadClass call

[glad cobalt]

Just doing it ourselves

[haughty copper]

to either remain in the TCL or pass it upwards

it's 5 lines of code in the loader 😄

which by the way

we somewhat alread have since we use a custom MCL

[glad cobalt]

Look I am going to be honest

There is no consensus on removing modules.

Not within the maintainers and not within the community

If you want to research further down this road

Then we should open this up for proper discussion

Instead of doing this in this thread

[glacial crag]

we are only discussing hypothesis here

[haughty copper]

Well, my current startup branch doesn't actually remove modules 😄

i commented out the boot layer hoisting to get ATM10 to launch

but the code is there to construct the structure as it is today

layer- and module-wise

[glacial crag]

the reality is that even in the maintainer team most people have no idea what module support actually entails

[glad cobalt]

Yes and that is fine you can keep on discussing hypothesis, but I think we all know it could work.

[haughty copper]

Well I am not sure we all know how little it'd change user-facing wise

but how much code we'd just delete

[glad cobalt]

That is fine, then make your case to all maintainers

Have them vote on it

[haughty copper]

Well in any case, about the CL

we currently have this code

[glad cobalt]

Is that not why we made neoforge

[glacial crag]

modules or not we need to think about classloaders

[glad cobalt]

So we can all have a say in these decisions

[chrome mortar]

modders will realistically not notice this at all, it's not like the modules are exactly visible to them anyway

[haughty copper]

That is in our code base (the package -> module lookup)

[glacial crag]

again, this is a technical discussion on details of the CL setup

that is not relevant for most maintainers (unless they have interest in it, of course)

[haughty copper]

There's no point in making a case without actually presenting a solution that works

Because functionally

There's not really anything that results from having modules

other than pain, currently

Which yes, we can work around with more work (see startup-experiments :P)

But the realization stands that being able to just delete large swaths of code without loss of functionality for our users

[chrome mortar]

hmm shartte, how exactly does your branch discover mc libraries

[glacial crag]

I am sure that it also has something that prevents the layers above the TCL from loading a transformable class

[haughty copper]

I am not sure it does 🤔

[glacial crag]

it 100% does, that's the entire point of classloader isolation

[haughty copper]

You can always go via ClassLoader.getSystemClassloader to get around it fully

it can't have something "in it"

Buuuuuut, we'll just find out

what they do

[glacial crag]

https://github.com/FabricMC/fabric-loader/pull/630

hmmm it is true that Fabric does not have intermediate class loaders

[haughty copper]

A side effect of this effort is that the entire game including libraries becomes exposed to Mixin and other bytecode editing.
And we do not actually want that, I hope? 😄

[glad cobalt]

No we do not

This was discussed a bunch

We especially do not want to expose auth libs and several others that are needed in that area

[glacial crag]

is there any reason besides performance?

[haughty copper]

I can primarily think of performance reasons

But those are imho quite significant

Since in our case, we parse them all

[glacial crag]

you can already mess with MC auth anyway since you can transform MC's source code

[glacial crag]

yes... 😒

in any case I don't think that there is demand for this feature

[plain solar]

it's a double-edged sword

on the one hand, maybe one day someone will want to patch a guava bug in an ancient version

but that seems unlikely, and there are probably hacks to pull it off anyway

in practice it seems unlikely that anyone cares about transforming any libraries except maybe DFU

[haughty copper]

So I'd just not open that can of worms

since it does have clear benefits of not

[plain solar]

that restriction also applies to libraries (not game libraries) loaded from mod jars, I assume

[glad cobalt]

Yes but that was always the golden rule when it came to core modding and mixin. An unwritten agreement with mojang, that we one do our stinking best to not make it easier, and two if we find people doing it report stuff like that to the community wide.

[glacial crag]

I'm not buying that since Fabric has been allowing it for a while

[haughty copper]

there's no reason to transform the auth lib when you can just jump over the call site

[glacial crag]

precisely

[haughty copper]

but again, i'd not spend any time on this

[glacial crag]

that's why auth is server-authoritative 😛

[haughty copper]

since we gain benefits from not even entertaining the thought

since it also means that in production, everything on the app cp is where it is intended to be with the exception of the three entrypoint classes

[glad cobalt]

I am not sure what to say to that, other then, it was agreed upon before you joined here.....

[glacial crag]

sounds like it's irrelevant to the present then 😛

[glad cobalt]

It does not seem like it to me

The rule was made for good reasons I am pretty sure, it extended beyond coremodding but more to a general principal

[haughty copper]

what rule

[glad cobalt]

The rule that we did not make systems that allowed bytecode modifications of mojangs authlib

I am not sure it still holds either

But I don't think it is wrong.

[haughty copper]

yeah that is very very weird

[glacial crag]

it's such a pointless rule anyway, who would come up with such nonsense

[haughty copper]

it kinda is though 😅

[glad cobalt]

The general thought is a good one

[haughty copper]

not really if you can hook into the game

[glacial crag]

you can redirect all calls to the authlib to a shaded modified version of it, very easily

[haughty copper]

the authlib doesnt run on its own

[plain solar]

pre-mixin that would have required some actual significant knowledge of bytecode

[haughty copper]

yes but so would have modifying the authlib

[glad cobalt]

It wa not that simple

[plain solar]

probably still does since mixins doesnt do global redirects

[haughty copper]

you dont have to do a global redirect

you know exactly where the call is in vanilla

this is like old-school securom hacking 😄

[glad cobalt]

My point is, preventing abuse and modifications of the auth systems is at least a worthy cause to a certain degree

[haughty copper]

when games really had a stupid if(!isValidCdInDrive()) { ERROR PIRATE! }

copy protection

if you can bytecode edit, you rewrite the if

not the isValidCdInDrive function

[glad cobalt]

I understand that with instrumentation you can basically do what ever you want

[haughty copper]

no not instrumentation

with just changing the game

[glacial crag]

I will say that with these "unwritten rules" that keep coming out of nowhere... IMO if there is no publicly accessible rule and/or clear reasoning that goes with it the rule does not exist

[glad cobalt]

I understand that, but suppose we had those protections in place

So that people could not easily modify those areas of the code

[haughty copper]

but we don't 😅

[glad cobalt]

Even then

No it was a hypothetical

as to the fact that protecting the auth chain is a reasonable idea

[haughty copper]

But anyway my opinion is that this discusison is a waste of time since the more libraries we keep on the app cp the faster it all is

[glad cobalt]

How much does it matter?

And what are the gains if we lift the libraries?

[haughty copper]

with our current transform chain

we still parse everything

with asm

so add a mountain of classes onto that...

[glacial crag]

classloading is still ridiculously slow (on NeoForge)

[haughty copper]

I stil have it in my head that we can at least try CDS at some point

that has massive perf benefits for java desktop apps normally but was so far outside of our reach

[glacial crag]

at least with parallel init phases we hide some of it

[glad cobalt]

CDS?

[haughty copper]

Class Data Sharing

[glad cobalt]

Ah yeah

I never heard that shorthand used for it

But tha makes sense

[haughty copper]

They generally improved it betwen 11 and 21 too, at this point is should be possible to have CDS archives for the "Prefix" of your actual classpath

which can still be used even if the classpath after it is appended

well anyway

In general using as much of the "standard stuff" as possible is appealing, in my opinion

but that is farther off and the startup-experiment benefits are more tangible and have nothing to do with actually dropping modules

[plain solar]

are you sure that is entirely our fault

when I profiled it last more and more relative time was being spent in java's own stuff

[haughty copper]

that's where CDS would help btw 😛

[plain solar]

I know we burn CPU cycles on the ASM parsing but I'm not sure that removing that will be a silver bullet

[glad cobalt]

Well I will leave you guys to this

[haughty copper]

the only consideration is if we keep the app cp where it is without modularizing it, which only works due to our lower layers not giving a fuck about require clauses heh

[haughty copper]

well no, keeping the libs where they are was about not adding more to it

[plain solar]

Isn't it, like, way past midnight there? 😅

[haughty copper]

since you'd then start parsing all of netty, guava, lwjgl, etc.

1:30 am

[glad cobalt]

Yeah

I was watching grey's anatomy with the wife

[haughty copper]

I am watching a dude rebuild a chateu

😅

[glad cobalt]

Oeh?

Also a good idea

[haughty copper]

embedded go and review the startup-experiments 😛

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

Specifically the stuff in here, which replaces BSL and rebuilds the module-path and makes us not need a legacyClassPath file anymore

https://github.com/neoforged/FancyModLoader/blob/startup-experiments/startup/src/main/java/net/neoforged/fmlstartup/Startup.java

[glad cobalt]

@haughty copper Are you using dynamic attach for the new FML?

Or how are you attaching the instrumentation

[glacial crag]

@timber ingot @glad cobalt the point of that PR is to make testing FML in dev and prod envs easier

[glad cobalt]

Yeah and we now have two maintainers that are not comfortable with that

Because FML is supposed to be detached from it

Dev testing is easy and does not need this PR