FML Clean-up

[haughty copper]

Ugh our "validations" there do not use translation keys

@glacial crag What about https://github.com/neoforged/FancyModLoader/pull/83/files ?

Obviously we're on fabric mixin now

But .... you made some other changes here

[glacial crag]

As long as we stay on fabric mixin we don't need that PR

Given current upstream progress we'll probably be on it for a long time

[haughty copper]

Then I'd suggest you close it 😄

[glacial crag]

Indeed I did

[haughty copper]

@chrome mortar ugh https://github.com/neoforged/FancyModLoader/pull/112/checks#step:7:108
How do we handle test reporting in GH actions?

FMLLoaderTest > WithoutMods > testNeoForgeDevClientDiscovery() FAILED
java.lang.AssertionError at FMLLoaderTest.java:130

And no attached artifact with the report doesn't help us

Unless I am missing something

[chrome mortar]

well not at all right now

rerun with debug might help?

[haughty copper]

isn't that just debug gh actions, not gradle --debug?

We might just wanna add artifacts **/build/reports/tests if we can

[chrome mortar]

the gradle action might be setting gradle flags if it detects action debug

not sure

so test it

[haughty copper]

Hm it didnt allow me to choose the debug flag

I thought there was a popup that asked for this

[chrome mortar]

there should have been one

[haughty copper]

Hm, it's failing only the NeoForgeDev cases 🤔

[chrome mortar]

it didn't help

ah well

[haughty copper]

Eh I can "fake" it by adding --info to the gradle tasks

well no it didnt run with the debug option or did you just do it again?

OH also, shouldn't it be running check?

It's currently running test

[chrome mortar]

FMLLoaderTest > WithoutMods > testNeoForgeDevServerDiscovery() FAILED
java.lang.AssertionError:
Expecting ArrayList:
["net/minecraft/client/Minecraft.class",
"net/minecraft/server/MinecraftServer.class"]
to contain only:
["assets/.mcassetsroot",
"data/.mcassetsroot",
"net/minecraft/client/Minecraft.class",
"net/minecraft/server/MinecraftServer.class"]
but could not find the following element(s):
["assets/.mcassetsroot", "data/.mcassetsroot"]

[haughty copper]

Yeah.... no idea

I've just installed J21 and checked out FML fresh in WSL2 on my machine and it ran successfully BUILD SUCCESSFUL in 54s

Nevermind. Idiot move.

Okay, same failure now. I can work with that

Aaaaahem

Why is SJH on Linux returning a base-path like that?

...... why does it do this ?!

it makes sense for entry, but it makes no sense for basepath

@glacial crag https://github.com/McModLauncher/securejarhandler/pull/70/files

[haughty copper]

Oh well userdev is completely busted 😄

[glacial crag]

hehehe

[glacial crag]

https://openjdk.org/jeps/8323658 it's coming soon™️

(JEP draft: Exception handling in switch (Preview))

[haughty copper]

Hm. We're scanning the system classloader for potential FML and ML services. So far so good.

Why do we also scan MOD_CLASSES in addition to that?

Shouldn't everything from mod classes already be on the system loader classpath?

[glad cobalt]

No.....

That is the funny thing

They way mod classes works

That is not actually required

And tools like workspace use it

[haughty copper]

Sorry, but what is "workspace"

and why can't it just be on the CP? 😛

It would make our lives easier I think

[glad cobalt]

Workspace is a tool by covers that creates an idea multi project environment that contains many mods service layer elements and neo. And runs without requiring the setup process by running on the native neo patched sources and binary class files

As such it does not run at all using the normal logic

And it combines the mods and service layers together into a run

Without them depending on each other

[haughty copper]

I don't like that we're cementing approaches that are even further away from how it runs in prod 😅

But blergh, so you're saying mod classes should be able to add to the CP, but we still scan the CP anyway

[mild storm]

Feels awfully weird to have your internal operations dictated by a third party tool..

[haughty copper]

We can trivially support that use case once we look at the startup process as a whole

currently it's messy since a very early call will already try to use MOD_CLASSES

before FML even really starts

Specifically ClasspathTransformerDiscoverer

[plain solar]

Wait, does this allow developing a mod and Neo simultaneously?

[glad cobalt]

Yeah

[plain solar]

That's insanely cool

[glad cobalt]

There are some issues

For example the mods ATs don't apply

[haughty copper]

Well okay, I did write a test case

[glad cobalt]

But yeah

[haughty copper]

/**
 * Special test-case that checks we can add additional candidates via the modFolders system property,
 * even if they are not on the classpath.
 */
@Test
void testUserdevWithModProjectNotOnClasspath() throws Exception {
    var additionalClasspath = installation.setupUserdevProject();
    var entrypointClass = SimulatedInstallation.generateClass("MOD_ENTRYPOINT", "mod/Entrypoint.class");
    var modManifest = SimulatedInstallation.createModsToml("mod", "1.0.0");
    var mainModule = installation.setupGradleModule(entrypointClass, modManifest);
    // Tell FML that the classes and resources directory belong together
    SimulatedInstallation.setModFoldersProperty(Map.of("mod", mainModule));
    var result = launchWithAdditionalClasspath("forgeclientuserdev", additionalClasspath);
    assertThat(result.pluginLayerModules()).doesNotContainKey("mod");
    assertThat(result.gameLayerModules()).containsKey("mod");
    installation.assertModContent(result, "mod", List.of(entrypointClass, modManifest));
}

[glad cobalt]

You basically load your mods into a neodev env

[haughty copper]

hm it stripped the blank lines. it's more readable in IDE 😛

Key point is that mainmodule (which are the simulated gradle output folders) do not get added to the additional classpath

And same testcase for the ModLauncher level classpath locator /sigh

[glad cobalt]

Yeah

This is really good work shartte

We need these kinds of tests for the ML-FML-NF chain rebuild anyway

[haughty copper]

What I cannot reliably test is the ML->FML handover yet

Essentially I start at the handover point to FML, but I have found a new way of skipping paths that have already been loaded by ML

[glad cobalt]

I would argue that something like that would need to happen in some e2E test variant somewhere

[haughty copper]

without having to exchange info between the ML transformer locators and the FML locators via static fields

If you do it E2E it becomes hard to inspect the final state properly, but yeah this is just so we have something in the FML project

currently we have absolutly no idea if the code in FML just crashes if used 😄

This at least gives us some coverage

BTW maybe you see something immediately wrong with this approach:

I am doing this when we enter the primary FML launch process:

// Index current layers of the module layer manager
for (var layerId : IModuleLayerManager.Layer.values()) {
    moduleLayerManager.getLayer(layerId).ifPresent(layer -> {
        for (var resolvedModule : layer.configuration().modules()) {
            resolvedModule.reference().location().ifPresent(moduleUri -> {
                try {
                    locatedPaths.add(Paths.get(moduleUri));
                } catch (Exception ignored) {}
            });
        }
    });
}
LOG.debug(LogMarkers.SCAN, "Located paths when launch context was created: {}", locatedPaths);

The idea is that a FML locator can then check if a jar it finds has already been picked up by ML

[glad cobalt]

Yeah

That should be fine

Allthough I I think you can just srhink that down to BOOT and PLUGIN

Cause the others are never present at that point

But I am not 100%

[haughty copper]

They are not, yeah

But this is nicely generic 😄

[glad cobalt]

Because FML is simultaniously a PLUGIN and the Bootstrap handler

So.....

Kind of depends on what ever you exactly mean with the Handover XD

[haughty copper]

Well yeah, pretty much when we enter FMLServiceProvider

At that point, ML already discovered some jars via ClasspathTransformerDiscoverer and ModDirTransformerDiscoverer, and built PLUGIN using those

So you're right, I could only scan PLUGIN

[glad cobalt]

IMHO that whole PLUGIN just needs to go.....

The concept that both do scanning

And both try to load the same thing

From the same location

Is just problematic

It runs into sooooo many issues

With duplicates alone we have a problem

Because every single locator needs to double and tripple check

But doing that means that at least some part of FML needs be on the boot layer and installed via the installer

Which is something I am totally fine with

[haughty copper]

Well, I agree with the first part, and we'll find a way around the second one ^^

But anyway, I need to find a way to actually test this at least once manually in all environments /sigh

[glad cobalt]

IMHO If we define a proper API for FML

Then I don't really think installing FML is a big deal

Especially since we always updated FML with MC

And I personally think we should retain that update cycle

[haughty copper]

p.s.: I also added support for setting MOD_CLASSES from either a system prop and also via a file

we'll see if we can somehow make NG simpler / more robust using that. don't know yet 👀

[haughty copper]

I'll get on integrating the PR into NF as well

[glad cobalt]

Very nice

[haughty copper]

Tech also didn't update to the latest FML before the locator PR 😄

[glacial crag]

I have it locally

[haughty copper]

The EventBusSubscriber thing?

[glacial crag]

Yes

[haughty copper]

Push it push it push it

I mean I can also fix it again in my PR but it'll just be noise in the diff

[glacial crag]

sec

hmmmmm

might need a full rebuild

[glacial crag]

fuuuuck

[haughty copper]

???

[glacial crag]

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

[haughty copper]

lol

That also needs a test in FML

[haughty copper]

@glacial crag This code is by you:

// Minecraft instance isn't available in datagen, so don't call initializeClient if in datagen
if (net.neoforged.fml.loading.FMLEnvironment.dist == net.neoforged.api.distmarker.Dist.CLIENT && !net.neoforged.fml.loading.FMLLoader.getLaunchHandler().isData()) {

In FluidType. Is this really necessary? Reaching back into the FML startup code?

[glacial crag]

probably from a reformatting commit though

that can be wired up as a DatagenModLoader check I think

[haughty copper]

Ah

Yes, that is MUCH saner

Although I don't like the codeflow much overall

[glacial crag]

oh yes that initalizeClient pattern will be removed eventually

[haughty copper]

Ugrhgluhg

[glad cobalt]

Maybe replace that immediatly with a Version or Constants class somewhere?

[haughty copper]

Yeah that's what I am also thinking

This is very weird data flow

since it's essentially CLI argument -> FML -> [used to find neoforge jar] -> then read inside of that very neoforge jar 😄

[glacial crag]

the neoforge version should be available from FML already?

[haughty copper]

Yeah as the version of the neoforge mod

[glacial crag]

why do we even have the spec and the group

[haughty copper]

As far as I can tell, it's only used to log on startup

In NeoForgeVersion:

        LOGGER.debug(Logging.CORE, "Found NeoForge version {}", neoForgeVersion);
        LOGGER.debug(Logging.CORE, "Found NeoForge spec {}", neoForgeSpec);
        LOGGER.debug(Logging.CORE, "Found NeoForge group {}", neoForgeGroup);

In NeoForgeMod:
CrashReportCallables.registerCrashCallable("NeoForge", () -> NeoForgeVersion.getGroup() + ":" + NeoForgeVersion.getVersion());

[glacial crag]

I'd just remove the spec and the group then

[haughty copper]

I'd keep the version, but get rid of the group, yeah

[glacial crag]

yes - keep version, remove spec, remove group is what I meant

and the version should be accessible via FML in all cases, it shouldn't be necessary to look at the manifest

[glad cobalt]

The group is actually needed I think....

[haughty copper]

Not there

[glad cobalt]

We used to use those in the past because of things like Sponge

Which build their API ontop of hours

And then modified

[haughty copper]

They have to add a sponge version info line regardless

But just to be sure, let me find some random sponge crash report to verify

[glad cobalt]

Sure

Just wanted to let you know

[haughty copper]

I thought about it, yeah. currently they'd already have to patch our jar in case they really want to replace the group there

[glacial crag]

Yeah they're not supposed to patch our jar anyway

[haughty copper]

Uh... what do we even print there 🤔

I can't even find that log string in my FC logs

Okay that group is always net.neoforged since we don't specify an implementation-title in our manifest anyway

[glad cobalt]

Okey great

Then lets rip it

And clean it

[haughty copper]

So I think it falls back to the hardcoded string 😐

... and the FML version is just bogus:

    FML: 20.4
    NeoForge: net.neoforged:20.4.196

[glacial crag]

that is computed by NG by trimming the neoforge version 😛

honestly the FML version is pointless anyway IMO

[haughty copper]

I mean if it says FML it should at least be the FML version 😄

[glacial crag]

I think that NG computes this by trimming the NF version

[haughty copper]

Ultimately we should merge https://github.com/McModLauncher/securejarhandler/pull/64 which would also make this unfeasible, but dunno if we'll manage for 1.20.5

[chrome mortar]

the spec version var is a bit flawed anyway

[haughty copper]

It just doesn't exist at all in "vanilla" JPMS

[chrome mortar]

iirc it also means that a neodev client is incompatible with a prod server

[haughty copper]

Not that - even if it did - it makes a ton of sense to log it

[chrome mortar]

or meant, not sure if we fixed it

[haughty copper]

The next freakshow class is NeoFormVersion 😄

[chrome mortar]

oh we're hosting a freakshow?

[chrome mortar]

why did no one tell me, i need to prepare the peanut gallery of interaction events

[glacial crag]

now that is completely useless as far as I know

(the neoform version in general)

[chrome mortar]

we should not even display it

[haughty copper]

That is also just logged

Essentially that entire class exists only for this log line:
LOGGER.info(NEOFORGEMOD, "NeoForge mod loading, version {}, for MC {} with MCP {}", NeoForgeVersion.getVersion(), NeoFormVersion.getMCVersion(), NeoFormVersion.getMCPVersion());

And this:

[haughty copper]

What is ForgeSnapshotsMod?

Interesting, I see

[haughty copper]

Okay nice, first attempt at neodev does launch with new FML 😄

https://github.com/neoforged/NeoForge/pull/834

[haughty copper]

@glacial crag https://github.com/neoforged/FancyModLoader/pull/112/commits/371762b6f11f51dd654ed6ed3585ecc818d0d29e thoughts?

[glacial crag]

Idk

Keep in mind that errors won't be found as long as CommonModLoader.begin is not called

So it would be wrong to check hasErrors in an early mixin for example

[haughty copper]

Hm?

Wdym by wrong though 😄

I mean compared to now

[glacial crag]

Well mixins should not check against the ModLoader

They should check the LoadingModList

If they get called early that is

[haughty copper]

Oh you just meant the comment

And BTW, that is kinda insane

[glacial crag]

Well ModLoader does things quite late

[haughty copper]

Yes, but the list of errors is available from multiple sources in different states

That is bad

[glacial crag]

Yeah

[haughty copper]

Hm. I don't understand. When running the runs in the Tests project, client-extra is not on the class path. (But on the legacy classpath)

This is confusing. In the main neoforge project, it is on both.

I suppose I'll have to restore FML reading the legacyClassPath system property until we can really dig into this 😐

[haughty copper]

Great

BSL currently loads the client-extra jar into a module

Hence my code excluded it in FML since it was already claimed/loaded

[glacial crag]

[haughty copper]

This ignoreList entry doesn't match the cilent-extra jar

[glacial crag]

what's the name of the client-extra jar?

[haughty copper]

client-1.20.5-rc1-client-extra.jar

At least in NF dev

[glacial crag]

yeah that doesn't work

[haughty copper]

it doesnt matter, the old code just looked in legacyClassPath for something containing "client-extra"

I'll restore that behavior and just somehow get it to work 😄

We'll clean this up for 1.21

[glacial crag]

yeah

do that

1.20.5 will probably release tomorrow

but it's ok if this takes a few extra days

[haughty copper]

I am just testing it manually now in all envs i can think of

and fixing bugs along the way

I was hoping we'd be able to drop legacyClassPath from FML but the NF environment is just... odd

For Tests: GameTestServer, client-extra is not on the CP

but it's on LCP

which imho should not happen

[glacial crag]

that is weird yeah

[haughty copper]

Even weirder it IS on the CP for the GameTestServer in the main project

[glacial crag]

[haughty copper]

Some weird NG thing probably. Dunno

Anyway, game tests pass again for NF dev

[haughty copper]

Alright. Userdev also works

But only showing warnings for unknown files in mods/ sucks, since I lost some time figuring out that my own mod still had a mods.toml 😐

So we might wanna expand that to candidates that come in via MOD_CLASSES

[glacial crag]

Yeah I agree

[haughty copper]

@glacial crag so I am refactoring the locators again to support better control over warnings

Ultimately, I'll remove the return value and instead pass in another "pipeline" argument

[glacial crag]

🤨

[haughty copper]

public interface IDiscoveryPipeline {

    Optional<IModFile> addPath(Path path, @Nullable IModFile parent);

    Optional<IModFile> addJarContent(JarContents contents, @Nullable IModFile parent);

    boolean addModFile(IModFile modFile);

    void addIssue(ModLoadingIssue issue);

}

I have yet to add the third arg to control:
REQUIRED
WARN_IF_INCOMPATIBLE
WARN_IF_SKIPPED
NO_WARNING

No idea how to phrase these yet 😄

[glacial crag]

what is happening 😄

[haughty copper]

What are you talking about 😄

[glacial crag]

why are there so many functions now 😛

[haughty copper]

I am removing IModFileProvider

That only existed because of the single return type function in the locator did not allow the locator to return mod-files at different levels of the pipeline

(the pipeline being: filesystem location -> jar contents -> mod file)

It also removes the entire reason I had to wrap the actual return type in a "DataResult": Stream<LoadResult<JarContents>>

Since I can just add issues directly via the pipeline now

[glacial crag]

I see

we probably want a standard helper for "try to add this path/jar content to the pipeline, if failed add an issue"?

[haughty copper]

Well, that's the other thing

We changed locator to only be able to return JarContent

But this way, a locator can also just add Path

and let the pipeline handle conversion into JarContent

And the pipeline will do what you describe (and auto-create the issue, centralize the exception handling for the Path->JarContent conversion, etc.)

[glacial crag]

ah ok nice

[haughty copper]

The only "issue" I still have: We have IModFile#getSource()

and if a mod file is located via JiJ, what is that supposed to return

[haughty copper]

I like this info output, but need to figure out why it finds mixinextras twice

[00:35:30.691] [main/INFO] [loading.moddiscovery.ModDiscoverer/SCAN]: Found mod file "server-1.20.5-20240423.152201-srg.jar" of type MOD [locator: production server provider +net.neoforged:neoforge:server:20.5.16-beta-pr-834-fml-locator-rewrite, system]
[00:35:30.700] [main/INFO] [loading.moddiscovery.ModDiscoverer/SCAN]: Found mod file "neoforge-20.5.16-beta-pr-834-fml-locator-rewrite-universal.jar" of type MOD [locator: PathBasedLocator[name=neoforge, paths=[libraries\net\neoforged\neoforge\20.5.16-beta-pr-834-fml-locator-rewrite\neoforge-20.5.16-beta-pr-834-fml-locator-rewrite-universal.jar]], reader: mod manifest]
[00:35:30.747] [main/INFO] [loading.moddiscovery.ModDiscoverer/SCAN]: Found mod file "mixinextras-neoforge-0.3.5.jar" of type GAMELIBRARY [parent: neoforge-20.5.16-beta-pr-834-fml-locator-rewrite-universal.jar, locator: jarinjar, reader: mod manifest]
[00:35:30.768] [main/INFO] [loading.moddiscovery.ModDiscoverer/SCAN]: Found mod file "mixinextras-neoforge-0.3.5.jar" of type GAMELIBRARY [parent: neoforge-20.5.16-beta-pr-834-fml-locator-rewrite-universal.jar, locator: jarinjar, reader: mod manifest]
[00:35:30.769] [main/INFO] [loading.moddiscovery.locators.JarInJarDependencyLocator/]: Found 1 dependencies adding them to mods collection

[haughty copper]

Alright tested in production client and production server.

Now gonna do the translations but we're closing in on this one

[haughty copper]

Alright, translations added. More tests added too.
This will require more testing and we'll likely uncover some bugs in configurations we did not think about.

[haughty copper]

Sooo, PRs ready. Now: protoyping time!

[glad cobalt]

Hmm?

[haughty copper]

I wanted to make the locator and java coremod stuff reviewable before starting to try some stuff for later

[glad cobalt]

Ah yeah

Do you have some time to chat in voice on the ModLocator stuff

?

[haughty copper]

I do not have a mike on my private PC 😅

I'd have to fish one out

Gimme a sec

[glad cobalt]

I need like a bit of time as well

[haughty copper]

Alright, let me know when

/sigh everytime I run spotless in FML, it changes ALL files from CRLF to LF 😐

Hm Orion, on second thought I am too lazy to go and find my mike. Maybe another time?

[glad cobalt]

Sure

[haughty copper]

Alright, I did a quick prototype: We could use a mini java agent to get access to Instrumentation, and use that to freely add opens/exports to java.base et al after the fact

Allowing us to essentially load our core modules later (as we do now), and add the exports/opens they need

This would - for example - solve our problem with opening up java.base to netty when we modularize it

Mock up:

class BootAgent {
    public static void premain(String agentArgs, Instrumentation inst) {
        BootAgentInterface.instrumentation = inst;
    }
}
---
public final class BootAgentInterface {
    static Instrumentation instrumentation;

    private BootAgentInterface() {
    }

    public static void addOpens(Module module, String pn, Module to) {
        if (instrumentation == null) {
            throw new IllegalStateException("The JVM was started without the BootAgent. Please make sure that the -javaagent parameter was not removed.");
        }
        // Do a caller check here to only make this callable from our own packages

        instrumentation.redefineModule(
                module,
                Set.of(),
                Map.of(),
                Map.of(pn, Set.of(to)),
                Set.of(),
                Map.of()
        );
    }
}

We could then call that BootAgentInterface from our normal startup code 🤔

[glacial crag]

Wtf

I'd just use --add-opens for the module controller and then we will be able to reflectively add exports to any module

[haughty copper]

That is the alternative

but that will open it up to all unnamed modules

[errant yew]

this code looks like something an overengineer would write

ship it

[glacial crag]

No, we can open to only the FML module

[haughty copper]

I'd actually try not to require the boot loader to be loaded as a module itself

[glacial crag]

Or to the bootstrapper I mean

[haughty copper]

Since it's really not necessary

Also, wdym wtf. This at least is supported 😄

[glacial crag]

It doesn't hurt though?

[glacial crag]

It's ugly as fuck 😛

[haughty copper]

What about it 😄

This is how mockito works 😄

[glacial crag]

They use a dynamic agent

[haughty copper]

They will not be able to soon

[glacial crag]

If we're gonna use a static one might as well add the opens directly

[haughty copper]

And their dynamic agent uses the exact same principle

The thing about the add-opens is that you still reach into internal methods

[glacial crag]

Isn't that what the agent would do too?

[haughty copper]

No?

The mechanism by which the agent adds-exports / add-opens is fully documented & supported

The add-opens/add-exports it adds themselves, THOSE are obviously reaching into internals

but that is not actually our code mostly

i.e. netty reaching into JDK internals 😄

[glacial crag]

Ahh there is an API for that, ok

[haughty copper]

yeah that redefineModule call is actually a public API

See what we get here: https://docs.oracle.com/javase%2F9%2Fdocs%2Fapi%2F%2F/java/lang/instrument/Instrumentation.html

In any case, I was just exploring the problem space 😄

[glacial crag]

Oh that is a nice API

[glacial crag]

I let a few comments on the PR

once they are sorted I'll do a bit of testing and then merge the PR

[haughty copper]

Thank you I'll go through them

p.s.: the trick to not loading transformers again is in LaunchContext#isLocated by scanning the previous layers

[haughty copper]

@glacial crag So i'll just remove the system mod flag on the mod-file level. Any mod-file can declare system mods via FML-System-Mods then

I also added a priority-system to locators/readers since they could snag the candidates from each other

Otherwise, review comments fixed, I'l re-request

[glad cobalt]

@haughty copper When do you have time to sit together.....

?

[glacial crag]

I think that we can just hardcode minecraft and neoforge as system mods

System mods = what we need to load to get to the error screen

[glacial crag]

I generally prefer text messages so that the people who are not in the call also get to follow the discussion. It's also too easy to say "I was in a call with X other people so now we'll do what we decided among us"

[haughty copper]

Out and about right now. So I don't know. What do you want to discuss? Is it.about the current PR? And yeah I like asynchronous comms more hehe

[haughty copper]

Yeah that would.go beyond. We would then do remove the manifest entry.from NF and didn't want to do that right now

[glad cobalt]

I understand that, but I am not a writing person. Especially not when I am initially formulating and transmitting ideas.

I am a fan of writing it down though once my idea is not stupid.

That way we can all work together on it

[glacial crag]

Well we can keep the manifest entry even if we don't use it anymore

[glacial crag]

Idk, I'm not convinced by that approach 😛

It doesn't seem to work that well

[glad cobalt]

I understand that

But that is how some people, like me work

I first want to bounce my ideas of someone with domain knowledege

I mean it could also be you

But shartte was working on it

[haughty copper]

Ya I understand. It's just that I usually do many things simultaneously and hopping in voice means I have to drop everything and focus on that for potentially hours 😅

Hence my hesitation

[glad cobalt]

Understood

I can do it with Tech as well

[haughty copper]

And my ADHD workflow is usually coding it until it works and I like it .

[glad cobalt]

I mean he has the same insights I guess

[glad cobalt]

That would normally be my way as well, but for this kind of piece of infrastructure, a bit of org and idea collection ahead of time, seems reasonable

[haughty copper]

We often forget some weird use case during theoretical discussion. The more we model in terms of tests or integration tests the easier.that workflow becomes 😅

[glad cobalt]

Yes for sure

[haughty copper]

Btw that is one of the reasons I would still be in favour or moving stuff to the NF repo. I have hopes this would enable one click e2e testing of a production like environment

Unless we can find some other way

Right now it's hard enough to check if NF makes use of some FML method hehehe

[glad cobalt]

Yeah

I might actually spin up a prototype at the end of the week

[haughty copper]

Sounds good now that I got sidetracked by neoform in a box lol

[glad cobalt]

My basic idea is to make ML a library and move the entry point entirely into FML

[haughty copper]

So many things to do 😩

[glad cobalt]

That is the other thing I need to do

Rip the pipeline stuff from NeoForm

[haughty copper]

That was my SECRET plan!!!!

[glad cobalt]

😛

Our ideas are not that stupid

The concequence is that FML needs to be installable (or some module from FML)

[haughty copper]

Because yes it's sooo much easier that.l way

[glad cobalt]

My plan was to make FML so small by moving most of it into NF

That updating it only on MC breaks is easy

[haughty copper]

We can keep a BSL style mini wrapper whose only purpose it is to load NF from the mods folder

[glad cobalt]

Nah

I don't want duplicate things

[haughty copper]

It would be version agnostic though

[glad cobalt]

I hate the fact that ML/BSL do mods dir locating for plugins

And that these locators work completely differently from FML locators

And behave completely differently

[haughty copper]

Yes but the rules are different too

At least in some.regards

[glad cobalt]

They don't need to be

That is the point

And right now that is the part that always introduced the most complexity for me

The idea that there are two locator types

Named the same

Scanning the same directory

[split wasp]

Could always take the approach quilt loader's loader plugins take -- plugins there are able to locate more plugins. So, in this world, you'd have a system for mod locators to be able to locate more mod locators

And then in theory it could all get moved to one sort of locator. Unless there's something besides locators that has to be discoverred that way -- I'm not familiar enough with all the bits and bobs of that system

[haughty copper]

Well yes but you need to find the first one

We want to install into the launcher version as little as possible

With the goal of having users only consciously install once per MC version

[split wasp]

You'd have to have 1 or 2 built in, yeah

And there's really no way around that

Because something has to start discovery

[haughty copper]

I thought about a bootloader that scans mods/* jars for a manifest attribute and loads a subfolder in those jars all into a boot layer

No complicated per file selection

[split wasp]

Seems like more complexity and duplication

[haughty copper]

We can not use JIJ as is imho since we otherwise cannot remove libs

If the user still has the old NF mod in their mods folder

[split wasp]

imo one should be able to JiJ locators

(And this is quite doable, in theory)

[haughty copper]

As a mod maybe but the boot system should be all or nothing as it is now

[split wasp]

Nah, I don't see a need for it to be

[haughty copper]

I do I don't wanna debug half of the system being loaded from nf-old.jar and the other half from nf-new.jar

[split wasp]

Why would you have two nf jars in the directory?

[haughty copper]

Have you met our userbase

[split wasp]

And the answer there is simple -- nf jars would pin the versions of specific things that ought not to be upgraded, so that would be an error case

As it ought to be

[haughty copper]

It should not be since I isn't for mods either

And happens a lot today

[split wasp]

It is for mods though? If I put two versions of the same mod in a directory it'll error

Won't it?

[haughty copper]

I don't think so

[split wasp]

Dear god, does that not error at present?

[glad cobalt]

No

It loads the newest

[split wasp]

That's... mildly concerning

[glad cobalt]

And that is on purpose

[haughty copper]

It's user friendly

[split wasp]

Does the oldest participate in jarJar selection at all?

[haughty copper]

It does

[split wasp]

Oh wow, that's atrocious. So you get a jar that is not itself loaded, but may have it's components loaded?

Ah well

If you expect mods to work fine with that, you should also expect NF to work fine with that

So I wouldn't see that possibility as a limit on recursive locator discovery

After all, anything NF JiJs, mods could also JiJ newer versions of

So the risk of loading half from one NF jar and half from another is equivalent to loading half from the NF jar and the other half from whatever mod bundles it

[haughty copper]

The problem I ran into conceptually with recursive locators is that an older version of X might be loaded already

And then a newer one comes along by a locator that was loaded later

[split wasp]

Fair enough

I'll think about that

[split wasp]

Going back to this -- this seems like an issue. If you're picking only the newer mod to load, the older mod shouldn't participate in jarJar version selection. Because, say, two different versions of the mod might pin a bunled dep to different versions. We really only want jars that are actually selected to have any of their children actually selected

[haughty copper]

We need to construct some test cases and d.efine the desired outcome

There are some.fucked up cases

[glad cobalt]

Regardless of jarJar

There is this issue with the following case:

[split wasp]

I'd almost phrase the problem in reverse. We want to take a full tree of jars and subjars, and trim out branches to leave no duplicates, preferring newer versions higher in the tree

[split wasp]

Could just be that selection between different versions of a locator is only allowed between locators at the same level -- if a locator was already loaded by something in a level closer to "root", it can't be loaded. Otherwise you end up needing a system to "unapply" loaders and that gets complicated

But I can also see how allowing recursive location of locators could add complexity

[haughty copper]

I mean mods loaded by locators

[split wasp]

Oh, sure, but that would presumably be figured out by the same sort of system that selects the newest version of multiple JiJed mods, right?

[glacial crag]

I don't think it makes much sense

[haughty copper]

I should clarify. Not mods. Services 😛

[glacial crag]

loading service-layer jars from mod jars just makes no sense

[haughty copper]

The case I thought of:
server pack locator is the only present file in mods/
it bundles earlydisplay

it then locates neoforge, which includes a newer version of earlydisplay

no dice

We already do that tech

[glacial crag]

since you need the service layer to exist to locate mods

[glacial crag]

sure, and it only works in hardcoded locations 😛

[haughty copper]

in mods/* 😄

The server pack locator use-case is hard.

It's just hard. 😄

[glacial crag]

I'm aware 😛

[plain solar]

it's the best use case

[split wasp]

Sure. My proposal was basically to have locators run first and be "special" and then you could treat everything else sensibly after that, but, I see your point. Basically -- my thoughts on this are that really, anything except a mod locator itself should definitely make sense to be locatable from jarJar-ed stuff

[haughty copper]

I should also clarify that we should write down the use cases, and those can become test cases later 😄

[split wasp]

Yeah that also makes a lot of sense

[plain solar]

there is a bootstrapping problem because things like GraphicsBootstrapper are not mod locators but run before mod locators

[haughty copper]

there are some pathological cases that I don't really care about but we should at least be aware of.

i.e. I think we currently determine the "file version" based on the "primary mod" in mods.toml (which just happens to be the first...)

[split wasp]

Ugh, that runs before mod locators?

[glacial crag]

of course it does since it can be used to show progress of the locators

[split wasp]

Yeah the whole thing with file versions is just weird

[glacial crag]

that "ugh" annoys me 😛

[haughty copper]

One of the experiments we should do: bring up a window using GLFW, render some shit, tear it down and then see if the Minecraft window opens up successfully... (on Mac)

[split wasp]

Locators ought to be fast enough that their progress shouldn't need to be shown, imo

[glacial crag]

have you ever heard of ServerPackLocator?

[haughty copper]

I was about to say 😄

[plain solar]

that will break our (Sodium/Embeddium's) use of GraphicsBootstrapper

that's the problem

[haughty copper]

And even normal locators on spinning disks surely exceed the normal rule of ~150ms without feedback

[plain solar]

I think mod locating takes a few seconds on my system, yes

[haughty copper]

on SPL there is seriously nothing you can do

[split wasp]

Anyways, in terms of file versions: If I stick two different LIBRARY files in the mods folder, that provide the same thing but with different versions -- how does FML figure that out? I've tested it and it seems to work, but what's it using to figure out that those're the same thing, and what the versions are, from the actual jar?

[haughty copper]

There is some hackish code that tries to parse it out of the filename

IIRC in SJH

[plain solar]

only a self-extracting service into mods so that it's picked up on the next launch, pretty much

[haughty copper]

Do you have a link to the hacks handy, embedded? Or would you need to search around for it?

[glacial crag]

well that's not the worst xD

[split wasp]

I feel like mod locating taking a few seconds is... more than a bit concerning. What's the bottleneck on it?

[split wasp]

That's a bit . Does it at least try to make use of the automatic module name and declared specification version or something? Now I have to check

[plain solar]

normally in vanilla/Fabric, the call to glfwCreateWindow is intercepted and before it runs, either an environment variable is set, or the process command line is changed () to instruct the Nvidia driver not to enable an optimization (the exact mechanism depends on operating system)

https://github.com/embeddedt/embeddium/blob/8bd1886755d7b8f95c23344969e1f22f00a55bdb/src/main/java/me/jellysquid/mods/sodium/client/compatibility/workarounds/nvidia/NvidiaWorkarounds.java#L15

on (Neo)Forge, this is currently a no-op because the window was already created at that point

[haughty copper]

Scanning hundreds of files?

Honestly embedded

That's it? 😄

Why isn't that just in NeoForge as a feature optin?

[plain solar]

because the implementation is horrendous (you're not exactly supposed to change the command line of a process in Windows at runtime, afaik)

[haughty copper]

But the more important question: does disabling threaded optimizations have any effect when embeddium is not present?

[plain solar]

I'm pretty sure it makes vanilla perform worse

could be wrong

[split wasp]

How much scanning is needed to construct the minimal info needed at that point though? In theory you just need to figure out what's in it and the version at that point, right? Or is there other work done then?

[haughty copper]

It only says should in the Windows API 😛

The lifetime of the returned value is managed by the system, applications should not free or modify this value.

[haughty copper]

Parsing the main zip file table of 150-200 files takes time if you have a spinning disk

Not to mention, we do it multiple times.

[split wasp]

I mean I know, one of my laptops has a spinning disk

[haughty copper]

@plain solar What irks me though is: what part of the cmdline is nvidia looking for. Is it that ugly-ass system prop mojang sets?

[plain solar]

I don't know if anyone knows for sure

at some point I believe JellySquid had reverse engineered one version of the driver to figure it out

but I also think the detection mechanism may have changed since then

The approach is not suitable for inclusion in Neo as-is, is my point

[haughty copper]

And is that nvapi that comes up on google not an option?

[plain solar]

iirc for Minecraft specifically the driver ignores a number of the reasonable ways of disabling this feature

[haughty copper]

If I read that API correctly it only looked like it was managing the profiles found in the control panel anyway, which sucks

[plain solar]

I think installing a profile was explored at some point and didn't work

[haughty copper]

Should just try to use ANGLE lol 😄

I wonder though, is it this setting? "-XX:HeapDumpPath=MojangTricksIntelDriversForPerformance_javaw.exe_minecraft.exe.heapdump"

Hm, probably not. It says Intel drivers

[plain solar]

they still ship that in 1.20.6?

[haughty copper]

I just pulled that from 1.20.4 at least

None of this is satisfactory, really

[plain solar]

for now the next version of Embeddium allows solving it with a manual edit of the FML config file

[haughty copper]

We need earlydisplay BEFORE location is done, really

[glacial crag]

ah we have priorities now?

[glad cobalt]

ELS needs to be started before locating

For me the order would be:

1. Main()
2. Locate platform (optionally if main is not in FML)
3. Start ELS
4. Start locating

It is a chicken and an egg problem

[glacial crag]

in the PR I mean

[glad cobalt]

Ah well then I have no idea

[glacial crag]

@haughty copper I'm happy with the PR; I will do a bit more testing right now, hopefully we can merge it afterwards

one question: priorities?

[chrome mortar]

our priority should be getting it shipped

[glacial crag]

I'm aware, that's why I am in this channel discussing the PR 😛

[chrome mortar]

anyway what exactly do you mean by priorities

parser priorities?

[glacial crag]

https://github.com/neoforged/FancyModLoader/pull/112/commits/907d9567e4c3a631a31ca1595bc64be95ba6948a

[chrome mortar]

oh yes that's a good idea

do you need convincing with an use case or

[glacial crag]

yes 😛

[chrome mortar]

simple one

[glacial crag]

I suppose it's mostly useful for mod file readers rather than locators

[chrome mortar]

a fabric mod adds a neoforge.mods.toml just so that people that try to run it on neo would get a "missing connector" error screen

but connector needs to be able to parse that file instead of neo as it's still a fabric mod

[haughty copper]

agreed

[glad cobalt]

The problem with that order shartte is that ELS becomes fixed

No more modifications by mods to ELS

[haughty copper]

Yuuuuuup, same thought here

[haughty copper]

Yes, true. For readers it's really required since they're first come first serve

For locators, there's the thing about ModDiscovery skipping paths that are already located

[glad cobalt]

Difficult situation to be honest

Custom locators are really usefull (at least can be, for packs etc)

The ability to modify the GL context is niche but used by some mods

Soooo

Not sure where to put that

[haughty copper]

For VERY early error -> AWT Error Dialog and System.exit (So, errors in completing 2))

That is if we cannot even find earlydisplay

[glad cobalt]

ELS would just always be in FML

[haughty copper]

But yes, the use case that embedded has... We really have no good option

The worst case scenario really is SPL

[glad cobalt]

So there would not even be discovery if main is held in FML

Yeah but I can think of other scenarios where it takes longer to locate

They all involve any kind of async operation

Or slower networking / disk

Then there is runtime patching

That will take time

So....

All of that

[glacial crag]

@haughty copper not quite there yet?

MCF is missing a lang entry

apparently the fabric one doesn't have the right context?

why is the button placed so horribly like that

might just be a bad merge on my end

[haughty copper]

Uh that button is not on me

Okay I gotta double-check the translation context

that entire system is an unmaintinable mess

[glacial crag]

it really is

just pad with nulls until the file is at position 2 I guess 😛

[haughty copper]

Urhgulhrgulh 😄

[bold prairie]

Platin probably missed that when they added the quit button

[lavish lantern]

yeah, looking at the PR code, I did miss it

I update the x position for one, but not the other

[glacial crag]

it's also weird that it says that 1 warning has occurred even though there's 2 of them 😛

[lavish lantern]

I didn't touch that ¯_(ツ)_/¯

[haughty copper]

Did you push your update to the branch, Tech?

[glacial crag]

no

I can

pushed

[haughty copper]

I added "for Minecraft Forge,"

since, people are just gonna stir up drama if a MCF mod gets deteted as "for an older version of NeoForge"

or did they change their mods.toml?

[lavish lantern]

y'all want me to quickly make a fix PR?

[glacial crag]

that's good

you should add a comma after NeoForge but that's a very minor nitpick

[glacial crag]

are you fixing the button placement too?

and if you could also check the displayed warning count... 😁

[haughty copper]

no 😄

[glacial crag]

then yes

[haughty copper]

BTW, I looked at the warning count

that was always broken and used the error count instead

and any count == 0 got clamped to 1 lol 😄

[glacial crag]

wtf

[lavish lantern]

alright, gimme a min

[haughty copper]

Well yes, the actual lang-entry hard codes it to 1 for <= 1, which is ok: {0,choice,1#1 warning has|1<{0} warnings have} occurred during loading

just not when it accidentally gets shown for count=0

[glacial crag]

what is this arcane syntax

[haughty copper]

Some apache common lang stuff? Maybe?

@glacial crag Okay, done https://github.com/neoforged/NeoForge/pull/834/commits/86251bf6f6f706ae6c2307560d563c76cd041311

[glacial crag]

I'll do more testing tomorrow then hopefully merge the PR

[haughty copper]

Aight

There will be bugs™️

[glacial crag]

oh yeah definitely

I just want to do a bit of low effort testing to make sure that we don't ship something too broken 😄

[haughty copper]

yup and it's much appreciated

[plain solar]

is the dirt background totally gone from vanilla code now? I much preferred it over this red... maybe a shade of brown would work better?

[haughty copper]

We're not gonna spend time tweaking backgrounds

At least not right now 😛

[chrome mortar]

okay i'm back home so what kind of testing do you need @haughty copper

[haughty copper]

Just running it I'd say

Userdev, NF dev with tests, prod

The like

[chrome mortar]

your PR is using the PR artifact build, right?

(neo PR)

yes it is

@haughty copper pls fix merge conflicts and bump the NG version for fixed installers™️

[haughty copper]

What did you do

[haughty copper]

Both done

[chrome mortar]

going great

[errant yew]

I love how most of that message is just the first line, with the extremely long command-line invocation

making it 18 KB in size

[chrome mortar]

Build file 'P:\NeoForged\NeoForge\projects\neoforge\build.gradle' line: 11

A problem occurred evaluating project ':neoforge'.
> Failed to write dummy data for dependency: ng_dummy_ng.net.minecraft:client:1.20.6:client-extra

i want to stab someone

like right now

[haughty copper]

Is this refactor related?

doesnt look like it

--fml.neoForgeVersion 20.6.3-beta-dmaps-updated 😄

[chrome mortar]

yeah i forgot to refresh gradle

now it is refactor related

so uhm @haughty copper it's obviously your fault

[glacial crag]

I don't think it is

That's BSL having a bad time

[chrome mortar]

ok so

i want someone to quickly PR a try catch there

i can debug it but it's not the first time i see the error, and if a modder encounters it too it will be painful to debug

java is also stupid anyway

[haughty copper]

hm, why?

is that likely used as a callback to native?

[haughty copper]

Not my fault 😄

[chrome mortar]

and it is indeed empty

anyway i still want that pr kthx

we really should have exception handling in all SecureJar#froms

so actually a try catch is better in another place

[glacial crag]

They should all throw IOException but alas

[chrome mortar]

checked exceptions are super annoying

[glacial crag]

No?

They exist so that people check them

Checked exceptions are a very nice feature IMO

[chrome mortar]

UnionFileSystem#openFileSystem needs to log the path when rethrowing the exception

[haughty copper]

IOException and InterruptedException are the only real "tolerable" ones

[errant yew]

i still do not quite understand how best to handle InterruptedException, to this day

[chrome mortar]

oh for fuck's sake, WHAT

something is cleaning the file

[haughty copper]

that makes two of us 😄

[chrome mortar]

shartte

[haughty copper]

Thread.current.interrupt() if you don't rethrow

[glacial crag]

Maybe you have two gradles

[haughty copper]

otherwise rethrow

[errant yew]

I thought you were commenting on my lack of skill thereof, maty

[chrome mortar]

can you try running the updated NG too

[haughty copper]

nice little spring cleaning

uh you mean in userdev?

[chrome mortar]

no

neodev

[chrome mortar]

that too

[haughty copper]

will try

[chrome mortar]

make sure to pull my commit updating the wrapper and gversion

[errant yew]

i feel like for some reason, NG (at least recently) is too keen on recreating ZIP files from scratch, even if they theoretically shouldn't be

[chrome mortar]

it is

[haughty copper]

neoform... in a box! 🗳️

i always wanted my setupDecompWorkspace back

Failed to write dummy data for dependency: ng_dummy_ng.net.minecraft:client:1.20.6:client-extra

[chrome mortar]

run the select task manually

then refresh

then try to run client

fail

run the select task manually

observe the file exists

run client

fail

[haughty copper]

rughrlugh. need to re-run setup

sec

[chrome mortar]

also look what i noticed randomly looking through NG code

[haughty copper]

oops.

Well it's a bit... we're only doing a joined dist anyway

the client seems to be a bit of a misnomer

So this could actually be correct

Depending on the context

[chrome mortar]

wtf the gradle task works

look what i tested without even wanting to

[errant yew]

that comma is placed incorrectly

also, that button overlap

[haughty copper]

The button overlap is from another PR that got already merged into 1.20.x

(and yet another PR shall fix it, but not this one!)

[chrome mortar]

wdym, it's fixed already

neodev works (the gradle task)

now to test a mdk

[haughty copper]

so far this has been a very long and painful road 😄

Also, yes same issue for me on neoforgedev, maty

Exception in thread "main" cpw.mods.niofs.union.UnionFileSystem$UncheckedIOException: java.util.zip.ZipException: zip END header not found
Caused by: java.util.zip.ZipException: zip END header not found
at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem.findEND(ZipFileSystem.java:1320)

[chrome mortar]

orion broke it

[glad cobalt]

What did I do?

[chrome mortar]

broke neodev

[glad cobalt]

How?

I will check it tomorrow

We have a bank holiday here

[chrome mortar]

MDK works

also @haughty copper want to know why pr publishing installers don't work?

[haughty copper]

the transitive PR repo is missing

I had to patch it by hand

(the installer json that is)

[chrome mortar]

vs

[chrome mortar]

that's not why

URI#resolve being annoying is why

[haughty copper]

lol

yeah welcome to me ~6 months ago

I discovered how fucking shite the Java URI class was back then

there are a ton of footguns with resolve especially

[chrome mortar]

    private static URI addLeadingSlash(URI uri) {
        String asString = uri.toString();
        if (asString.endsWith("/")) return uri;
        return URI.create(asString + "/");
    }

[haughty copper]

kinda, but it only works since we dont have a fragment

or query string

[chrome mortar]

maven urls don't have query strings

[haughty copper]

I know it's just not a general purpose "resolve this relative path against this url"

[chrome mortar]

prod works

[chrome mortar]

yes but for this use case specifically it works

@haughty copper https://github.com/neoforged/NeoGradle/pull/159

i understand about nothing of this ng code

[haughty copper]

Waiting for tests to complete then I'll merge it. Why do we not have the auto-merge on that repo?

[glacial crag]

it needs the branch protection to be setup first

[chrome mortar]

added

[jovial vault]

Leading I think you mean Trailing ;P

[errant yew]

smh maty

[chrome mortar]

oh lol

[errant yew]

@chrome mortar wouldn't using the process-test-data job for the status check still mean it will always pass

because the job is set to if: success() || failure()

[chrome mortar]

https://github.com/neoforged/NeoGradle/pull/140/files#diff-59673315318021940521301545d2d77d184143947c225efdd58b03efc15cde1eL167-L180 well this is the problem

[chrome mortar]

yes

NG has historically had failing tests

[errant yew]

[chrome mortar]

and with it using +... i doubt it won't have in the future

if we always delete and recreate the file not only do we cause needless disk strain but also introduce more random errors with concurrent invocations

which IDEs do do

[errant yew]

bring it up in #neogradle-dev

for more eyes

[chrome mortar]

#builds message and orion disabled my correct auto merge and instead enabled it with the default commit message

[prisma elk]

[Jump to referenced message](#builds message) in #builds

matyrobbrt referenced · Apr 30, 2024, 8:53 PM

NeoGradle

Build Started

Version

7.0.117

Build Branch

NG_7.0

Commit message

Fix installer URLs without a trailing slash (#159)

• Fix installer URLs without a leading slash

URI#resolve is stupid.

• I meant trailing trust me

Quoted | Matyrobbrt · Apr 30, 2024, 8:53 PM

[errant yew]

[glacial crag]

so is this usable or is NG still preventing it?

[glad cobalt]

I am opening my IDE

I just finished eating

my breakfast

What need fixing

[prisma elk]

matyrobbrt

[Reference to](#neogradle-dev message) #neogradle-dev [➤ ](#neogradle-dev message)@glad cobalt why exactly did you change that

technici4n referenced · Apr 30, 2024, 8:44 PM

[chrome mortar]

https://github.com/neoforged/NeoGradle/pull/161

[chrome mortar]

@glacial crag you have 3 hours to test the fml pr more if you want

[prisma elk]

Successfully scheduled reminder on <t:1714579535:F> (<t:1714579535:R>)!

[glacial crag]

sounds good

[haughty copper]

Is this sufficient, @chrome mortar ? Or did I forget something?

The overall goal of this PR is to make the code structure in the FML mod discovery process easier to follow and to support a better end-to-end error reporting experience for mod loading issues of all kinds.

To this end, the PR rewrites the mod discovery system with the following changes:

• Remove superfluous API surface
• Add test coverage to the mod discovervy process by trying to simulate our common installation environments (Production, NeoForge Developoment, Mod Development)
• Unify the reporting of mod loading errors and warnings on a single type ModLoadingIssue, and provide a single exception that can report one or more such issues up the call chain (ModLoadingException)
• Allow locators to produce output at different levels of the discovery pipeline:
  • Unprocessed filesystem locations (i.e. from files in the mods folder)
  • Virtual Jar Files combined from various file-system locations and filtered (i.e. userdev locator that splits the combined Minecraft+Neoforge Jar into multiple separate virtual Jar files)
  • Fully built mod files (such as the Minecraft mod-file for which the locator provides the mod metadata on-the-fly)
• Allow locators to also supply mod loading issues (warnings and errors) on a per-file basis
• Introduce a priority system for locators and readers, allowing user supplied implementations to go before/after the built-in ones
• Untangle the dependency between locators and launch targets by having launch targets supply additional locators that are specific to the environment they are concerned with (i.e. the production launch targets supply locators for the production client/server)
• Untagle the dependency on the early phase ModLauncher discovery by determining whether a file has already been loaded not by reaching into previous locators statically, but rather by inspecting the ModLauncher ModuleLayers that have already been built when FML starts mod discovery

[glacial crag]

that looks very good

[chrome mortar]

fine by me

next up do that sjh logging change while I test a bit more

[haughty copper]

Should I also approve the FML PR 😄

[glacial crag]

lol

[prisma elk]

@chrome mortar

Camelot

Reminder

merge fml pr

matyrobbrt · May 1, 2024, 4:05 PM

[chrome mortar]

gotta wait a second, i'm adding that logging line to sjh

[haughty copper]

When you say logging line, do you actually mean to say add the failing path to the exception message?

[chrome mortar]

yes

@haughty copper https://github.com/McModLauncher/securejarhandler/pull/71 i'm waiting 😛

[haughty copper]

Where is the PR description 😇

public UncheckedIOException(final String message, final IOException cause) eclipse user detected

or where are the finals coming in

Also. What the fuck is this private static class UncheckedIOException extends java.io.UncheckedIOException {

[errant yew]

what

ah, no-oping fillInStackTrace

[chrome mortar]

my hand?

@haughty copper did you never update sjh in the fml pr?

@glacial crag reapprove

[glacial crag]

why strikethrough?

[chrome mortar]

the 3.0 SJH bump broke a test

[glacial crag]

that and we don't dismiss approvals for FML apparently

did something else change in SJH?

[chrome mortar]

shartte pr'd something

[glacial crag]

this? https://github.com/McModLauncher/securejarhandler/commit/312eb116454c220a51332616a717adf0d117955f

so @chrome mortar are you fixing it?

[chrome mortar]

I need to figure out why it broke first

[glacial crag]

you mind if I try to fix it?

[chrome mortar]

oh this is sneaky

anywho think i fixed it, let me run tests

[haughty copper]

What was it?

[chrome mortar]

you turned the bipred<str, str> into a unionfilefilter

[glacial crag]

btw this is suspicious

[chrome mortar]

that's exactly the issue tech

lol

[glacial crag]

hehehe

[chrome mortar]

the tests are running, this should fix it

in the meantime someone should write a quick announcement

"hey your locators are broken"

[haughty copper]

Ah yes, the PR to SJH was in response to the FML locator

since string copmarisons on the base path are fucking stupid

[chrome mortar]

@glacial crag approve

[glacial crag]

approved

[plain solar]

so exactly how much carnage is this PR going to bring when it's merged

welp, asked too late

guess I'll find out

[haughty copper]

@plain solar in all honestly, probably not a lot

There might... no there certainly are still bugs. But there aren't that many custom locators out there, and adapting is not that hard.

[chrome mortar]

@haughty copper you have checks fialing

on neo

[haughty copper]

I know i know

Error: Exception in thread "main" java.lang.module.ResolutionException: Module JarJarMetadata reads more than one module named cpw.mods.securejarhandler

Welcome to dependency hell

Why did you have to go and update that SJH dep, Maty? 😄

Whyyyyyy 😱

[chrome mortar]

Hey everyone!
We have recently merged a PR rewriting the FML locator system, and it has been released as NeoForge version 20.6.14-beta, affecting only 1.20.6.
This update is more technical than developer-facing, but it will break any mods adding custom locators. However, mods for another loader (including mods using an old mods.toml file) will now be correctly detected and warned about, in a way that will not give mods such as Connector headaches.

This is quite a big update, so please do not hesitate to report any bugs you find.

[haughty copper]

Make sure to announce it as affecting 1.20.6 only

more prominently than just the version

[glacial crag]

ok apparently you can't even sort mods within a single mods.toml declaration?

the declaration is done at the ModInfo level but the sorting happens with ModFileInfos???

[haughty copper]

Uh what do you mean?

What I do wrong

[glacial crag]

I don't think it's your fault

[haughty copper]

Especially since we use the first mod from mods.toml as the "primary mod"

[glacial crag]

the sorting is done across files instead of being done across mods 😐

yet the dependency declaration is per-mod

[plain solar]

uhhh

got this after updating to 20.6.18 in my dev env

the actual cause of the error is unrelated

but why aren't these translated?

[haughty copper]

There is an error during an event!

[glacial crag]

hehehehehe

ordering!!

[haughty copper]

Oho! are you doing that as an FML test? 😅

[glacial crag]

yeah 😛

I did think about having a test for this inside of FML, but I didn't want to be mocking everything

[chrome mortar]

@haughty copper re #124, I am very much leaning towards closing as not planned

[haughty copper]

I'd like to understand why Gradle is not collapsing the dep

and instead decides to include it multiple times hm

We can pull it from the legacy class path again, which is why this was not happening before

[restive stag]

The issue is that Gradle has no configuration which adds to runtimeClasspath and compileClasspath WITHOUT being transitive

which means that you need to declare a custom configuration in order to use NeoForge currently

[chrome mortar]

that doesn't make leaking neo deps any better 😛

and this people is why NGs rule is stupid but I digress

[restive stag]

I feel that you and @glad cobalt need to duke this one out to figure out what you actually want to do 😛

[haughty copper]

Is it bad?

Because your mod does depend on neoforge

why should it not declare it?

[restive stag]

because as it stands currently, you must declare a custom configuration to "correctly" consume NeoForge

which is a lot of work that wasn't previously necessary

[haughty copper]

Didnt we do all this dep replacement dance so the normal mechanics of gradle would work?

[restive stag]

so either NeoGradle should supply a configuration which does this work, or the locator just doesn't throw for duplicates

[haughty copper]

meaning if you depend on 2 projects which themselves depend on net.neoforged:neoforge:x

it would resolve it so you only have that dep ONCE

Not throwing for duplicate is not an option, it would have to be switched over to using the legacy classpath hack

since if we have MC on the classpath from across multiple projects, it could pick the one with the wrong ATs applied

i seriously did not account for this case happening because it seems like a NG bug

[restive stag]

the issue is, to my understanding, that because the artifacts are different, they get all get passed together

[haughty copper]

(or me misundestanding basic gradle)

[restive stag]

the repos are also different, to my understanding

[haughty copper]

yes but that shouldn't matter? for external deps you dont know the repos they originally used at all

[restive stag]

that is, one mod project's ng_dummy_ng repository is different from another mod project's ng_dummy_ng repository

¯_(ツ)_/¯

[glacial crag]

imagine if we had a proper configuration for neo stuff that didn't get leaked 😛

[haughty copper]

Gradle was a mistake™️

[restive stag]

I think neogradle prepends the ng_dummy_ng prefix to the group to force it to lookup from the repo it made

or something to that effect

[chrome mortar]

pipe dream innit

[restive stag]

I'm not too sure

[haughty copper]

Presumably yes, but the dependency in the other subproject has the same prefix

[glacial crag]

cause right now it's an "implementation" dep until it isn't lol

should it be added to an equivalent of localRuntimeOnly

[haughty copper]

I really think we should fix the root cause of this if we actually can 🤔

The thing is: for subprojects you dont want the dep to leak, for transitive deps on other mods you actually might

[glacial crag]

isn't leaking the dep always incorrect?

[chrome mortar]

why should a ng_dummy_ng dep leak

[haughty copper]

They do depend on it

[restive stag]

yeah, I theorised another workaround which would be:

configurations {
    whyNeoforge { }
    compileClasspath.extendsFrom(whyNeoforge)
}

runs.configureEach { run ->
    run.dependencies {
        runtime confgurations.whyNeoforge
    }
}

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

[haughty copper]

oh

wait

[glacial crag]

what??

[haughty copper]

it leaks the replaced dependencies?

[glacial crag]

that's not gonna work don't even bother lol

[haughty copper]

not the original one?

[chrome mortar]

uhm that's going too far

[glacial crag]

pretty sure yes

[chrome mortar]

just add to runtimeClasspath

[haughty copper]

blergh that sucks

[glacial crag]

what part of replacement don't you understand 😛

[haughty copper]

all of it

[glacial crag]

hehe ok; I can say I understand 1% now

[chrome mortar]

tech the question is what part you understand and the answer is "si pronto"

[restive stag]

wouldn't only adding to runtimeClasspath mean you can't compile against it?

[haughty copper]

the top or the bottom 1%? 😇

[glacial crag]

idk 😄

[haughty copper]

"any" 1%

[chrome mortar]

to both classpaths

[haughty copper]

so essentially you did an any % speedrun

[chrome mortar]

the whole runtime thing is not needed

[haughty copper]

And yes blergh that is incredibly bad that it leaks the replaced deps

[restive stag]

right, that's good

i wasn't super sure at first so i played it safe

Gradle™️

[haughty copper]

what would be beneficial is if it exposed net.neoforged:neoforge:20.6.100 so gradle picks the highest required version across the set of dependencies

[glacial crag]

yes

but how exactly does that work when the dep is literally getting replaced?

[chrome mortar]

in theory

[restive stag]

at the very least, I think we got the "workaround" working for soaryn's stream :p

[chrome mortar]

ng_dummy_ng speaks for itself.. "oh ng, you dummy..." /s

also people need to stop proxying soaryn, just get the guy to ask for support himself in the official servers and repos lol

[restive stag]

i don't disagree buuut this also affects me

[glacial crag]

I don't think that's feasible

[restive stag]

and soaryn probably won't ever change because, y'know

that's just how some people be

[chrome mortar]

leaking mc deps has historically been bad™️ although that was mostly because mappings affected the version

[glacial crag]

if you add neoforge to compileOnly and localRuntimeOnly you should be fine

I think

[restive stag]

localRuntimeOnly? that exists?

[glacial crag]

in that case it shouldn't leak to other projects

[glacial crag]

only in the MDK

[chrome mortar]

it doesn't, it's part of the mdk

[restive stag]

ah, TIL

[chrome mortar]

ideally ng would have a config for this, but this isn't an ideal world..

[glacial crag]

we'll have to reconsider that stance because it's quite ridiculous IMO

[restive stag]

well, please do let me know when you've decided what the actual solution should be

[glacial crag]

the solution for now is to use these configurations

you can copy the definition of localRuntimeOnly from the MDK

we will look for a better fix on the NG side whenever we can

[restive stag]

yeah

[haughty copper]

So do we however for the time being make the locator use the legacyClasspath?

[glacial crag]

please no

we have uncovered broken behavior in NG / whatever people are doing

let's not add a workaround that will stay with us for years

[restive stag]

at the very least, documenting this somewhere outside of this thread would be very helpful, which is also part of the reason I opened the github issue

because I imagine it's a good number of people who currently have this "issue" 😛

[haughty copper]

I really don't wanna fuck over modders until we get around to fixing NG 😄

[glacial crag]

the response needs a minimum of gradle understanding unfortunately

[haughty copper]

Since from a modders perspective, implementation dep on NF is the correct intent

[glacial crag]

I do; otherwise NG will never be fixed and FML will keep that crappy hack forever

[glacial crag]

implementation leaks the dependency to consumers 😛

[chrome mortar]

you see, I look at this issue as living proof that NGs design concepts are flawed

it's a motivation to get them changed :P

[haughty copper]

can you when you do replacement, replace the replaced dependencies in another configuration?

TBH we should drop the fancy idea of adding it to implementation altogether I assume, we'll never get this to work right

if dependency replacement cannot add dependencies to other configurations than the one the original dependency is in

[glacial crag]

the entire point of dep replacement is that it's local to the configuration

[split wasp]

Dep replacement should be yeeted

What's the advantage over another system? That you can have multiple neo versions in a project and it's easier for non-main source sets? Cool, we can do that without dep replacement...

We already link stuff to a source set when we do dep replacement (and runs and stuff pull that), so it's not true dep replacement anyways

Might as well just go all out and fully link it to a source set

[glad cobalt]

Can i get a tldr on what is the issue. Cause the GitHub issues on this are unclear. And the suggestions are weird. We already do the split replacement. It is the main reason dep replacement exists in this weird way per configuration. It can't leak to consumers cause if you add it to implementation it is not actually added to that configuration but to complieClasspath and runtime classpath instead. Even in multi project workspaces Gradle does not leak them through.......

Is the ide being stupid here?

Using your own configurations does not help here then....
Because it would still need to end up in runtime classpath and compile classpath for it to work....

I guess it is because they are different paths...

And that causes issues

Because that will create duplicates at runtime in ides

Hmm

Not sure how Gradle handles that internally

That might indeed be a design flaw

But has no bearing on anything you all discussed

[glad cobalt]

Actually it is not, I checked how FG6 handled this.

And there you also get two jars in the CP (well not if you had exactly the same setup, but two different ATs already sufficed, and is not preventable!)

I think we can collapse them trivially if they are actually 100% the same

And then we should

But you will not get around the problem of handling this natively in FML somewhere

The IDEs will always pass in two Minecraft instances

[glad cobalt]

That is literally what it already does.....

https://github.com/neoforged/NeoGradle/blob/NG_7.0/userdev/src/main/java/net/neoforged/gradle/userdev/dependency/UserDevDependencyManager.java#L54

https://github.com/neoforged/NeoGradle/blob/NG_7.0/dsl/common/src/main/groovy/net/neoforged/gradle/dsl/common/util/ConfigurationUtils.groovy#L65-L120

So it literally already does what you are wanting it to do

[glacial crag]

"it is not actually added to implementation but to compileClasspath and runtimeClasspath"
How is that intuitive in any way? The entire argument for dependency replacement was that it's supposed to be more or less obvious

On top of that, the dependency replacement code is one of the worst pieces of code in the entire NeoForged project. It's completely unreadable.

It's not obvious at all what that configuration does in the subsequent Gradle and lambda soup

[glad cobalt]

It is not..... It is the concequence of luke not wanting my stupid pom hack from NG6 and FG6.

[glad cobalt]

Sure it needs cleaning

And that can be done relatively easily

But in this particular case it is a straw man argument, cause cleaning up that code, won't solve your issue with multiple Forges beeing in the classpath.

Which is a problem of the FML rewrite combined with how IDEs process dependencies with the same GAV but in different locations. Gradle will just pass one as far as I can see, while the IDE will pass them all (Aka Gradle differentiates by GAV, while the IDE differentiates by path).

This in practice is not solveable by NG7, because as I already said, you can easily get multiple distinct NeoForge jars by using different ATs (something very common in multi-loader project, or even combined workplaces). This was already the case in FG5+, and the old FML handled this gracefully, which clearly indicates that this is a regression of some kind in FML.

That said, I agree that I need to rewrite thet dep replacement logic, to make it cleaner and better documented.

[glacial crag]

At that point we might as well just have a DSL to select the NeoForge version and get rid of replacement

[glad cobalt]

Yet there were several people in the community which wanted different NF versions per sourceset

For what ever fucking reason

[glacial crag]

And that can be done with a DSL too

[glad cobalt]

The dep replacement (outside of the code being attrocious) is not the problem here.

So can we please get back to fixing the problem at hand, instead of drifting of into topics which have nothing to do with it?

I understand that you find it complex, ugly and weird, and your complaint is noted and will be addressed in due time

But we have a bigger problem to solve right now

You all stated you don't think this is an FML issue, however I can clearly show that even with all of your wishes completly full filled, in multi-project layouts it is trivial to get a duplicate NF or minecraft jar

Is that sufficient to call it a regression in FML?
Or do we need to discuss this further?

[glacial crag]

I'm not sure I understand how the dep is leaking to other projects

[haughty copper]

So wait. If you have the ability to add deps to other configurations based on dependency replacement, why do we not add a configuration that does not leak outside of the project

[glad cobalt]

It is not