Custom Gradle plugin for NeoForge itself | The NeoForged Project | Page 2

supple maple Nov 14, 2024, 8:32 PM

#

fork

tiny flare Nov 14, 2024, 8:42 PM

#

1: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':neoforge-coremods:spotlessJava'.
> Error while evaluating property 'lineEndingsPolicy' of task ':neoforge-coremods:spotlessJava'.
   > Spotless JVM-local cache is stale. Regenerate the cache with
       rmdir /q /s .gradle/configuration-cache
     To make this workaround obsolete, please upvote https://github.com/diffplug/spotless/issues/987

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.
> Get more help at https://help.gradle.org.
==============================================================================

#

WHAT THE ACTUAL FUCK

#

I hope it's not that issue

#

of course it's that issue

#

https://github.com/diffplug/spotless/issues/987

GitHub

Petition Gradle to support JVM-local configuration-cache · Issue #9...

Please thumbs-up this issue if you'd like Gradle to build this. TLDR: Unlike remote build cache, configuration cache is never going to be relocatable. Because of that, it might as well be a JVM...

#

wtf is this seriously

#

prod server installs and boots 🎉

tiny flare Nov 14, 2024, 9:27 PM

#

so I am diffing the .module file

#

why were we adding the neoform zip and the neoform tools to modDevRuntimeElements?

west torrent Nov 14, 2024, 9:35 PM

#

There was some reason 🤔 (SRG related, IIRC)

#

tools not so much

#

but data

tiny flare Nov 14, 2024, 9:38 PM

#

apparently gradle dependencies are written in Asturian

craggy shadow Nov 14, 2024, 9:45 PM

#

tiny flare https://github.com/diffplug/spotless/issues/987

Ah yes, classic spotless. Want me to open a PR to use my replacement?

tiny flare Nov 14, 2024, 9:46 PM

#

I think you already opened a PR 😅

craggy shadow Nov 14, 2024, 9:46 PM

#

tiny flare apparently gradle dependencies are written in Asturian

I was getting that on random class files a while back. Specifically asturain, that is. IDK the cause...

craggy shadow Nov 14, 2024, 9:47 PM

#

tiny flare I think you already opened a PR 😅

Ah yeah, https://github.com/neoforged/NeoForge/pull/1530

craggy shadow Nov 14, 2024, 9:59 PM

#

tiny flare ``` 1: Task failed with an exception. ----------- * What went wrong: Execution f...

Also, I'm sorry, I don't think I'd seen that particular message before -- you're telling me that spotless... just "worked around" config cache by making it so that you have to run something to clear the cache manually if the daemon is restarted? By attaching something to the currently running JVM or some shit? What?

#

Goddamnit, every time I think it can't get worse, it gets worse

tiny flare Nov 14, 2024, 10:09 PM

#

craggy shadow Also, I'm sorry, I don't think I'd seen that _particular_ message before -- you'...

yes lol

#

it's horrendous

#

"please go clear your configuration cache now" is just ridiculous

#

ok I will remove the neoform zip from the userdev libraries (it's already available under the mcp key)

#

if NG still works after that then it means it wasn't necessary

supple maple Nov 14, 2024, 10:14 PM

#

contrary to popular belief, the spotless code is not spotless

west torrent Nov 14, 2024, 10:32 PM

#

tiny flare ok I will remove the neoform zip from the userdev libraries (it's already availa...

might have been for the obf-utils? FML at some point wanted the neoform version and looked it up from the libraries

craggy shadow Nov 14, 2024, 10:45 PM

#

tiny flare yes lol

Every time I think I've seen the worse spotless has to offer, I am proven wrong...

tiny flare Nov 14, 2024, 10:59 PM

#

west torrent might have been for the obf-utils? FML at some point wanted the neoform version ...

ah yeah maybe

#

I wonder why this happens 🤔

#

maybe junit or opentest4j requires apiguardian

#

where is opentest4j even coming from

#

userdevTestClasspath
\--- net.neoforged.fancymodloader:junit-fml:5.0.1
     +--- org.junit:junit-bom:5.10.2
     |    +--- org.junit.platform:junit-platform-launcher:1.10.2 (c)
     |    +--- org.junit.platform:junit-platform-engine:1.10.2 (c)
     |    \--- org.junit.platform:junit-platform-commons:1.10.2 (c)
     \--- org.junit.platform:junit-platform-launcher -> 1.10.2
          +--- org.junit:junit-bom:5.10.2 (*)
          \--- org.junit.platform:junit-platform-engine:1.10.2
               +--- org.opentest4j:opentest4j:1.3.0
               +--- org.junit.platform:junit-platform-commons:1.10.2
               |    \--- org.apiguardian:apiguardian-api:1.1.2
               \--- org.apiguardian:apiguardian-api:1.1.2

#

I suppose that api guardian is compile only

tiny flare Nov 15, 2024, 7:24 AM

#

alright the PR works with both MDG (basic runs) and NG (I checked all runs & unit tests)

shell dove Nov 15, 2024, 7:56 AM

#

wait the PR is not to replace NG with MDG but so neodev works with either?

supple maple Nov 15, 2024, 7:56 AM

#

he's talking about userdev

shell dove Nov 15, 2024, 7:56 AM

#

oh

tiny flare Nov 15, 2024, 8:30 AM

#

The PR replaces NG by a custom plugin

#

It's a very different situation than just replacing with MDG

west torrent Nov 15, 2024, 10:36 AM

#

*in neodev only

#

It moves / refactors most of the neodev specific code from NG to buildSrc in the neoforge repo

#

and reuses parts of MDG to decompile minecraft / set up IDE runs

west torrent Nov 15, 2024, 10:44 AM

#

tiny flare I wonder why this happens 🤔

What is that list actually used for? NG only?

tiny flare Nov 15, 2024, 10:49 AM

#

yes

#

we might have a similar situation with the outgoing configuration, but I did not check that

west torrent Nov 15, 2024, 10:54 AM

#

hm, depends on the attributes, but yeah

west torrent Nov 15, 2024, 1:35 PM

#

// Modules are on the classpath too but should be ignored by BootstrapLauncher
var modulePathIgnoreList = configurations.modulePath.getIncoming().getArtifacts().getResolvedArtifacts().map(results -> {
    return results.stream().map(r -> r.getFile().getName()).toList();
});

#

I am not even sure we need that anymore. I did add code to BSL at some point to skip CP entries that correspond to already loaded locations

tiny flare Nov 15, 2024, 2:26 PM

#

yeah you're right it might just work without this

#

will try at some point

west torrent Nov 15, 2024, 9:05 PM

#

@tiny flare you do not have installerConfig as part of your configurations collection class, is that intentional?

#

Same for some of the installer tools

tiny flare Nov 16, 2024, 12:16 AM

#

Yes same as the jst configs

#

And AT and NFRT

#

They are IMO self contained to the point where moving them would make things less readable

tiny flare Nov 16, 2024, 12:17 AM

#

west torrent Same for some of the installer tools

All of them no?

west torrent Nov 16, 2024, 12:25 AM

#

yeah

tiny flare Nov 16, 2024, 12:30 AM

#

Same reasoning

#

They're self contained so no need to track them centrally

tiny flare Nov 16, 2024, 4:20 PM

#

west torrent I am not even sure we need that anymore. I did add code to BSL at some point to ...

it works without in dev

west torrent Nov 16, 2024, 4:22 PM

#

and in prod?

tiny flare Nov 16, 2024, 4:25 PM

#

I'll tell you in a bit 😄

#

is this useful in any way?

#

@west torrent thoughts?

west torrent Nov 16, 2024, 4:42 PM

#

even with colors

#

Does gradle do the (R) and (C) to denote resolvable and consumable?

#

Or am I imagining that

tiny flare Nov 16, 2024, 4:44 PM

#

#

I can add it

#

I used the eyedropper to have the same colors 😄

west torrent Nov 16, 2024, 4:50 PM

#

your arrays are also the wrong way around

#

and we should add a small legend, the arrow means "extendsFrom", right?

tiny flare Nov 16, 2024, 4:52 PM

#

west torrent your arrays are also the wrong way around

ah oops 😄

#

the ordering is a bit shitty

supple maple Nov 16, 2024, 5:01 PM

#

those arrows aren't nice to follow

tiny flare Nov 16, 2024, 5:01 PM

#

yeah it kinda sucks

#

well anyway, if someone wants to play around with this:

flowchart RL
    classDef dependencyScope fill:#66cd00,color:#000
    classDef consumable fill:#ffc0cb,color:#000
    classDef resolvable fill:#add8e6,color:#000

    implementation:::dependencyScope

    neoFormData:::dependencyScope
    neoFormDependencies:::dependencyScope
    libraries:::dependencyScope
    moduleLibraries:::dependencyScope
    userdevCompileOnly:::dependencyScope
    userdevTestFixtures:::dependencyScope

    neoFormDataOnly:::resolvable
    neoFormClasspath:::resolvable
    modulePath:::resolvable
    userdevClasspath:::resolvable
    userdevCompileOnlyClasspath:::resolvable
    userdevTestClasspath:::resolvable
    launcherProfileClasspath:::resolvable

    implementation --> libraries & moduleLibraries & neoFormDependencies
    neoFormDataOnly --> neoFormData
    neoFormClasspath --> neoFormDependencies
    modulePath --> moduleLibraries
    userdevClasspath --> libraries & moduleLibraries & userdevCompileOnly
    userdevCompileOnlyClasspath --> userdevCompileOnly
    userdevTestClasspath --> userdevTestFixtures
    launcherProfileClasspath --> libraries & moduleLibraries

    modDevBundle:::consumable --> neoFormData
    modDevApiElements:::consumable --> libraries & moduleLibraries & userdevCompileOnly & neoFormDependencies
    modDevRuntimeElements:::consumable --> libraries & moduleLibraries & neoFormDependencies
    modDevModulePath:::consumable --> moduleLibraries
    modDevTestFixtures:::consumable --> userdevTestFixtures

#

I think it sucks atm

#

maybe all the resolvables could be moved to one side and the consumables to the other

#

idk

tiny flare Nov 16, 2024, 5:06 PM

#

west torrent and in prod?

yes!

#

ok, change made

west torrent Nov 16, 2024, 6:13 PM

#

@tiny flare do we have a util for snake to camlcase?

#

sorry, underscore to camel

#

I'd want to implement this in Tools:

/**
  * The name of the Gradle {@link org.gradle.api.artifacts.Configuration} used to resolve this particular tool.
  */
 public String getGradleConfigurationName() {
     
 }

#

We will ultimately create a resolvable config for every tool

tiny flare Nov 16, 2024, 6:13 PM

#

In MDG yes

#

You can copy-paste it

west torrent Nov 16, 2024, 6:14 PM

#

Okay but nothing in the guaranteed gradle classpath?

#

Even Commons StringUtils doesnt seemt o have it

#

I'll google once more

#

No we'd need commons text, ok.

tiny flare Nov 16, 2024, 6:21 PM

#

Yeah and I don't want it 😄

#

Same reason why I wrote it again for MDG

west torrent Nov 16, 2024, 6:22 PM

#

I didn'T find it in MDG
I'll just make the config name a property of the enum

#

and pass it in the enum ctor

tiny flare Nov 16, 2024, 6:22 PM

#

I was thinking of https://github.com/neoforged/ModDevGradle/blob/6f1f3785a54f81b25867e8857c23968c9e6e39c8/src/main/java/net/neoforged/moddevgradle/internal/utils/StringUtils.java#L34

west torrent Nov 16, 2024, 6:23 PM

#

That is just capitalize, we have that in commons lang3

#

I was talking

AUTO_RENAMING_TOOL - > autoRenamingTool

#

To get autoRenamingToolClasspath as the configuration name

tiny flare Nov 16, 2024, 6:23 PM

#

Ah yeah 🤷

west torrent Nov 16, 2024, 6:23 PM

#

Too much work

#

#

This also makes it easier to grep those names

#

A bit sad, to actually add the deps I also need to use Project internally in your neat configuration class ^^

tiny flare Nov 16, 2024, 10:28 PM

#

Ohno what are you doing 😄

west torrent Nov 16, 2024, 11:15 PM

#

Nothing fancy. Hm, in which cases do we make these configs non-transitive?

#

Generally we run them as executable jars meaning the configuration must only be a single file

west torrent Nov 17, 2024, 12:35 AM

#

Hm, the tasks are not well grouped

#

in the gradle table I mean

west torrent Nov 17, 2024, 1:29 AM

#

I set a bunch of task groups:
I am not sure what else would go into the "top level"?

We should also probably move the actually relevant MDG tasks into that group

#

(runClient etal)

tiny flare Nov 17, 2024, 7:52 AM

#

west torrent Nothing fancy. Hm, in which cases do we make these configs non-transitive?

The installer processors need to be transitive

#

I don't think moving these configs to the configs class is a good idea because

They are already self-contained wherever they are declared
They don't follow declaration / resolution separation

#

We can have a helper method to create a self contained config in NeoDevPlugin imp

west torrent Nov 17, 2024, 10:04 AM

#

Define "self contained" please

tiny flare Nov 17, 2024, 10:15 AM

#

Does not inherit from or extend from other configurations

west torrent Nov 17, 2024, 10:16 AM

#

Hm, why would you only put those in the NeoForgeConfigurations class 😄

tiny flare Nov 17, 2024, 10:17 AM

#

No I would NOT put them there

#

It just moves their declaration away from their usage with little added value

west torrent Nov 17, 2024, 10:18 AM

#

Tools can be (and are) reused though

#

We can revert that btw but I don't think the configuration naming for resolving tools was really all that consistent

#

And I actually liked the centralized location to look up configurations 😛

#

BTW because I just noticed it:

FileUtils.writeStringSafe(
        getInstallerProfile().getAsFile().get().toPath(),
        new GsonBuilder().setPrettyPrinting().disableHtmlEscaping().create().toJson(profile),
        // TODO: Not sure what this should be? Most likely the file is ASCII.
        StandardCharsets.UTF_8);

JSON is always UTF-8

#

Huh, I thought it actually said that in the standard 🤔

#

I suppose it's just by convention

#

Okay, it's just the associated Internet RFC that specifies it as UTF-8

#

8.1. Character Encoding

JSON text exchanged between systems that are not part of a closed
ecosystem MUST be encoded using UTF-8 [RFC3629].

west torrent Nov 17, 2024, 10:29 AM

#

tiny flare The installer processors need to be transitive

Can you explain why that is the case? I do not see us specify class-names anywhere, how does that even work?

tiny flare Nov 17, 2024, 12:32 PM

#

Look at the installer profile json

#

The tool gav is added directly, as well as the full classpath for the tool

west torrent Nov 17, 2024, 12:45 PM

#

PFffpfpfpfp ok

#

I find that annoyingly modeled then

west torrent Nov 17, 2024, 2:11 PM

#

Okay, I did add ignoreTransitive to Tools, the question I have is: for which tools do we even need to enable it?

#

Isn't the need to enable it an indication that the tools fatjar has been improperly published?

tiny flare Nov 17, 2024, 4:11 PM

#

Why would you NOT accept transitive deps?

#

Worst case getSingleFile will fail

#

It's better than getting a random runtime crash

tiny flare Nov 17, 2024, 5:02 PM

#

transitive = false:

binpatcher - Used to resolve the tool for creating binary patches
\--- net.neoforged.installertools:binarypatcher:2.1.2

transitive = true:

installerProcessorBINPATCHER
\--- net.neoforged.installertools:binarypatcher:2.1.2
     +--- net.neoforged:srgutils:1.0.0
     +--- net.sf.jopt-simple:jopt-simple:5.0.4
     +--- com.github.jponge:lzma-java:1.3
     +--- com.nothome:javaxdelta:2.0.1
     |    \--- trove:trove:1.0.2
     \--- net.neoforged.installertools:cli-utils:2.1.2

#

it does depend on net.neoforged.installertools:binarypatcher:%s:fatjar thonk

#

art - Used to resolve the jar remapping tool
\--- net.neoforged:AutoRenamingTool:2.0.3
     +--- net.sf.jopt-simple:jopt-simple:6.0-alpha-3
     +--- org.ow2.asm:asm:9.5
     +--- org.ow2.asm:asm-commons:9.5
     |    +--- org.ow2.asm:asm:9.5
     |    \--- org.ow2.asm:asm-tree:9.5
     |         \--- org.ow2.asm:asm:9.5
     +--- org.ow2.asm:asm-tree:9.5 (*)
     +--- net.neoforged.javadoctor:gson-io:2.0.17
     |    +--- net.neoforged.javadoctor:spec:2.0.17
     |    \--- com.google.code.gson:gson:2.10.1
     +--- net.neoforged:srgutils:1.0.0
     \--- net.neoforged.installertools:cli-utils:2.1.4

installerProcessorFART
\--- net.neoforged:AutoRenamingTool:2.0.3
     +--- net.sf.jopt-simple:jopt-simple:6.0-alpha-3
     +--- org.ow2.asm:asm:9.5
     +--- org.ow2.asm:asm-commons:9.5
     |    +--- org.ow2.asm:asm:9.5
     |    \--- org.ow2.asm:asm-tree:9.5
     |         \--- org.ow2.asm:asm:9.5
     +--- org.ow2.asm:asm-tree:9.5 (*)
     +--- net.neoforged.javadoctor:gson-io:2.0.17
     |    +--- net.neoforged.javadoctor:spec:2.0.17
     |    \--- com.google.code.gson:gson:2.10.1
     +--- net.neoforged:srgutils:1.0.0
     \--- net.neoforged.installertools:cli-utils:2.1.4

#

what the hell 😄

#

ah here we set the main class directly heh

tiny flare Nov 17, 2024, 5:07 PM

#

west torrent Isn't the need to enable it an indication that the tools fatjar has been imprope...

yes totally

#

probably because it is being consumed through maven metadata instead of gradle metadata

west torrent Nov 17, 2024, 5:22 PM

#

You mean we disabled publishing that?

#

Possibly

west torrent Nov 17, 2024, 6:49 PM

#

Trying to add a bit of javadoc

west torrent Nov 17, 2024, 9:16 PM

#

Yo but Tech, overall: great job! I already like a lot how easy it is to know where to look when looking at buildSrc

#

Further simplification goes beyond the scope of buildSrc imho, such as unifying the dev/userdev launch targets, etc. etc.

west torrent Nov 17, 2024, 9:42 PM

#

I am considering changing the PR title

#

"Use an in-tree Gradle plugin instead of an external Gradle Plugin to make version-specific changes easier" ?

tiny flare Nov 17, 2024, 9:42 PM

#

Yes

tiny flare Nov 17, 2024, 9:43 PM

#

west torrent Okay, I did add ignoreTransitive to Tools, the question I have is: for which too...

Right now it's false for all

#

Try running setup and build

#

Something should fail, no?

west torrent Nov 17, 2024, 9:43 PM

#

Yes, that's my thought

#

I will investigate the build failure 😄

#

Anyhow here's probably some of the last bits of cleanup before we should solicit reviews
https://github.com/neoforged/NeoForge/pull/1485/commits/a33b3a249af71334927f339a3c04375de99e4fcb

#

I renamed WriteUserDevConfig -> CreateUserDevConfig to be in-line with the CreateInstallerProfile/CreateLauncherProfile tasks

#

Yeah the binpatcher is borked

* What went wrong:
Execution failed for task ':neoforge:generateClientBinPatches'.
> No main class specified and classpath is not an executable jar.

#

BTW I really quickly would like to move the task from FML over that allows us to test the installer we create

#

With an in-tree task

#

I will check why binpatcher is borked

tiny flare Nov 17, 2024, 9:53 PM

#

Just specify the main class

#

Or make the config nontransitive. (Will that work in the installer though?)

tiny flare Nov 17, 2024, 9:53 PM

#

west torrent BTW I really quickly would like to move the task from FML over that allows us to...

Maybe yeah

west torrent Nov 17, 2024, 9:56 PM

#

tiny flare Or make the config nontransitive. (Will that work in the installer though?)

I mean it did work when it was non-transitive, I'll check the publishing though

#

The metadata looks fine on both central and our maven

#

@tiny flare we're using 2.1.2, which isn't published to Maven central and has no Gradle metadata on our own Maven either 😐

#

Hm no, that's not true, it has the metadata in 2.1.2 too

west torrent Nov 17, 2024, 10:29 PM

#

Is it just ignoring the classifier? 🤔

#

This really just seems badly done on Gradles part unless I misunderstand.

west torrent Nov 17, 2024, 11:31 PM

#

So if you specify a dependency on a classifier, does it just stop using the module metadata?

#

The obvious places in their docs refuse to mention this

#

Yeah, setting

spec.attributes(attr -> {
    attr.attribute(Bundling.BUNDLING_ATTRIBUTE, project.getObjects().named(Bundling.class, Bundling.SHADOWED));
});

Seems to do the trick

craggy shadow Nov 17, 2024, 11:54 PM

#

west torrent So if you specify a dependency on a classifier, does it just stop using the modu...

It uses the module metadata still. But it can't use dependencies from there because the classifier isn't part of the metadata, so it resolves the variant as if there were no classifier, then replaces it's artifacts using the classifier

#

In other words, classifiers are only part of artifact selection, not component (or variant!) selection

#

So whatever variant you'd have resolved without the classifier, those're the dependencies you get

#

(same with extension)

west torrent Nov 18, 2024, 12:03 AM

#

That does not seem like the most sensible option but even then I could not find that behavior in their docs

#

Well it may be somewhere but as usual that'd be well hidden

west torrent Nov 18, 2024, 12:27 AM

#

Tech I set the requested bundling to SHADOW on all tools, but now I suppose I gotta go and re-check the created installer profile

#

What's also annoying is that it seems undocumented how the compatibility matrix for Bundling actually looks like

craggy shadow Nov 18, 2024, 1:17 AM

#

west torrent That does not seem like the most sensible option but even then I could not find ...

There's no better one, unfortunately, with how classifiers work

#

(How they work in maven, that is)

#

Since in maven (and the only reason gradle has classifiers is what ivy/maven do... gradle's own features model stuff far better), when you depend on stuff, the dependencies only care about the scope, not the classifier -- aka an -api jar and normal jar have the same dependencies at compile scope

#

Among other reasons, this is why you should never use a classifier-based dependency when the relevant jar is actually properly declared in the gradle module metadata by attribute or classifier -- you could bring in the wrong deps, and even if you don't, the artifact is attached to the wrong variant (hence wrong attributes, classifiers, etc. which could matter to tasks consuming that information).

#

...I do wish gradle warned you about that, or had a warning you could turn on about that, but I guess there's enough cases where that could be desired (i.e., wanting to publish proper maven metadata or whatever).

#

Maybe I should make a plugin full of, like, validation to avoid gradle footguns and give nicer errors for wacky stuff or whatever...

west torrent Nov 18, 2024, 1:56 AM

#

Or you know, they might have just used the variants that have the file it substitutes anyway, if they're present

craggy shadow Nov 18, 2024, 2:39 AM

#

west torrent Or you know, they might have just used the variants that have the file it substi...

Uh... yeah no, that'd be awful. Because, you know the whole idea of a "classifier" isn't actually encoded anywhere in a variant

#

Plus, that would violate how variant resolution works -- you'd have a dependency which best matches one variant, but then has to pick another

#

In other words, it'd pull artifact-level concepts up into variants resolution, when those're really separate (and for "normal" gradle setups, where variants can have more than one thing attached or whatever, they have to be)

#

Furthermore, it adds a ton of complexity to proper cross-project deps

#

Basically, at some points, requires a variant to keep information about its classifier on it

#

Which... just doesn't make sense since for maven metadata, classifiers are entirely separate from the metadata, and you can't get a list of valid ones, so behaviour would be inconsistent in terms of whether the variant-classifier matches the artifact-classifier

tiny flare Nov 18, 2024, 7:56 AM

#

Another way to look at the problem is to see that maven doesn't allow for per-artifact dependencies

#

I am hesitant to use Gradle variants because we do need the installer to download stuff, possibly using a GAV to do do

west torrent Nov 18, 2024, 9:27 AM

#

tiny flare I am hesitant to use Gradle variants because we do need the installer to downloa...

Good point actually. Bah. So back to disabling transitive?

tiny flare Nov 18, 2024, 9:37 AM

#

nah just try it

#

after all we check all repos to find the artifact right

#

as long as we provide the right URL to the installer I think it's fine?

west torrent Nov 18, 2024, 9:37 AM

#

Kinda, yes. The issue is if someone loses the fatjar qualifier on the Tools

#

it'd now work in Gradle

tiny flare Nov 18, 2024, 9:38 AM

#

is the fatjar qualifier even in the installer json

west torrent Nov 18, 2024, 9:38 AM

#

but probably not when it tries to resolve gav->url

#

No idea 😄

#

I had planned to compare the resulting JSONs now

#

once more

tiny flare Nov 18, 2024, 9:38 AM

#

yes it is

west torrent Nov 18, 2024, 9:38 AM

#

after these changes

tiny flare Nov 18, 2024, 9:38 AM

#

    {
      "jar": "net.neoforged.installertools:binarypatcher:2.1.2:fatjar",
      "classpath": [
        "net.neoforged.installertools:binarypatcher:2.1.2:fatjar",
        "net.neoforged:srgutils:1.0.0",
        "net.sf.jopt-simple:jopt-simple:5.0.4",
        "com.github.jponge:lzma-java:1.3",
        "com.nothome:javaxdelta:2.0.1",
        "net.neoforged.installertools:cli-utils:2.1.2",
        "trove:trove:1.0.2"
      ],
      "args": [
        "--clean",
        "{MC_SRG}",
        "--output",
        "{PATCHED}",
        "--apply",
        "{BINPATCH}"
      ]
    }

#

that's what I have before your changes 😛

west torrent Nov 18, 2024, 9:38 AM

#

I know i know, I wanted to do it again

#

just against the same NF version

tiny flare Nov 18, 2024, 11:00 AM

#

@west torrent why are we even requesting fatjars?

#

wouldn't it be better to always request slim jars?

#

it just means that we'd have to hardcode the main class in the plugin

paper wadi Nov 18, 2024, 11:04 AM

#

tiny flare it just means that we'd have to hardcode the main class in the plugin

Why? You could just have the main-class attribute on the slim jar too

tiny flare Nov 18, 2024, 11:06 AM

#

west torrent Yeah the binpatcher is borked ``` * What went wrong: Execution failed for task ...

^

#

only the legacyinstaller needs to be a single-file because it gets included in the installer jar

#

everything else can safely have multiple jars

paper wadi Nov 18, 2024, 11:18 AM

#

Classpath is not an executable jar? What

#

What does that even mean

#

The classpath isn't a JAR

tiny flare Nov 18, 2024, 11:20 AM

#

if the classpath of the JavaExec task is a single jar then it is used as the main jar

west torrent Nov 18, 2024, 11:57 AM

#

paper wadi Why? You could just have the main-class attribute on the slim jar too

The JavaExec Gradle task can only run executable jars if the classpath consists of exactly one item

#

(which is very annoying!)

paper wadi Nov 18, 2024, 11:59 AM

#

Oh

#

WTF

tardy pebble Nov 18, 2024, 12:00 PM

#

https://docs.gradle.org/current/dsl/org.gradle.api.tasks.JavaExec.html

task runExecutableJar(type: JavaExec) {
  // Executable jars can have only _one_ jar on the classpath.
  classpath = files(tasks.jar)

  // 'main' does not need to be specified

  // arguments to pass to the application
  args 'appArg1'
}

checks out

raven copper Nov 18, 2024, 12:00 PM

#

concern

tardy pebble Nov 18, 2024, 12:01 PM

#

likely it's because you can't specify a 'primary' jar to run in JavaExec, unlike with java -jar <jar> -cp <classpath>

fathom plume Nov 18, 2024, 12:08 PM

#

that seems like a really annoying oversight of JavaExec

paper wadi Nov 18, 2024, 12:18 PM

#

I would have thought they'd check the classpath jars

#

And if they find one and only one with main-class use that

west torrent Nov 18, 2024, 2:55 PM

#

tardy pebble likely it's because you can't specify a 'primary' jar to run in `JavaExec`, unli...

That doesn't actually work I think

#

anything after -jar is passed as program args

#

I am not even sure you can specify a classpath when you specify an executable jar, but might be mistaken

tardy pebble Nov 18, 2024, 2:58 PM

#

you might be right, after taking a peek at https://docs.oracle.com/en/java/javase/23/docs/specs/man/java.html

#

yea,

#

-jar jarfile
Executes a program encapsulated in a JAR file. The jarfile argument is the name of a JAR file with a manifest that contains a line in the form Main-Class:classname that defines the class with the public static void main(String[] args) method that serves as your application's starting point. When you use -jar, the specified JAR file is the source of all user classes, and other class path settings are ignored. If you're using JAR files, then see jar.

args ...
Optional: Arguments following mainclass, source-file, -jar jarfile, and -m or --module module/mainclass are passed as arguments to the main class.

craggy shadow Nov 18, 2024, 4:34 PM

#

tiny flare I am hesitant to use Gradle variants because we do need the installer to downloa...

Can translate the one consumed by variant into a GAV with a classifier for the installer though, right?

#

Same as MDG does for the artifact manifest for NFRT

tiny flare Nov 18, 2024, 4:37 PM

#

Yes it can be done

#

Right now it's not done for the main jar GAV which is leading to a mismatch

#

Right now I am questioning why we would ever want to use a fatjar

tiny flare Nov 18, 2024, 5:36 PM

#

I will change all descriptors to use slim jars

#

except for LegacyInstaller

#

ah no, we should use the shadowed jars because they have minimize() set to true on them

#

ART does at least

#

then why is it so fat thonk

#

> Could not resolve all files for configuration ':neoforge:toolJstClasspath'.
   > Could not find net.neoforged.jst:api:1.0.45.
     Searched in the following locations:
       - https://maven.neoforged.net/releases/net/neoforged/jst/api/1.0.45/api-1.0.45.pom
       - https://repo.maven.apache.org/maven2/net/neoforged/jst/api/1.0.45/api-1.0.45.pom
       - file:/C:/Users/bruno/.m2/repository/net/neoforged/jst/api/1.0.45/api-1.0.45.pom
     Required by:
         project :neoforge > net.neoforged.jst:jst-cli:1.0.45
   > Could not find net.neoforged.jst:parchment:1.0.45.
     Searched in the following locations:
       - https://maven.neoforged.net/releases/net/neoforged/jst/parchment/1.0.45/parchment-1.0.45.pom
       - https://repo.maven.apache.org/maven2/net/neoforged/jst/parchment/1.0.45/parchment-1.0.45.pom
       - file:/C:/Users/bruno/.m2/repository/net/neoforged/jst/parchment/1.0.45/parchment-1.0.45.pom
     Required by:
         project :neoforge > net.neoforged.jst:jst-cli:1.0.45
   > Could not find net.neoforged.jst:accesstransformers:1.0.45.
     Searched in the following locations:
       - https://maven.neoforged.net/releases/net/neoforged/jst/accesstransformers/1.0.45/accesstransformers-1.0.45.pom
       - https://repo.maven.apache.org/maven2/net/neoforged/jst/accesstransformers/1.0.45/accesstransformers-1.0.45.pom
       - file:/C:/Users/bruno/.m2/repository/net/neoforged/jst/accesstransformers/1.0.45/accesstransformers-1.0.45.pom
     Required by:
         project :neoforge > net.neoforged.jst:jst-cli:1.0.45

so... the jst-cli publication is fucked

#

we have to keep the fatjar binpatcher because that's what's used in the userdev jar

#

lucky I noticed blob_sweat

#

shite

#

client installer works

#

server installer is broken again

#

😒

#

-DignoreList=

#

noooo not this crap again

#

if the ignore list is empty it ignores everything kek

#

oops, that's my mistake from the past

#

or maybe that's not the issue? idk

tardy pebble Nov 18, 2024, 6:29 PM

#

tiny flare if the ignore list is empty it ignores everything <:kek:1134098707869073408>

wait, what

tiny flare Nov 18, 2024, 6:29 PM

#

amazing

tardy pebble Nov 18, 2024, 6:29 PM

#

kek

tiny flare Nov 18, 2024, 6:30 PM

#

        var ignoreList = System.getProperty("ignoreList", "asm,securejarhandler");
        var ignores = ignoreList.split(",");

#

I guess this is intended behavior of String.split?

#

jshell> "".split(",")
$1 ==> String[1] { "" }

#

yeah

tardy pebble Nov 18, 2024, 6:31 PM

#

https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/String.html#split(java.lang.String,int)

If the expression does not match any part of the input then the resulting array has just one element, namely this string.

tiny flare Nov 18, 2024, 6:32 PM

#

hmmmyeah

#

makes sense in general I suppose, it's just weird for an empty string

tardy pebble Nov 18, 2024, 6:33 PM

#

well, probably need to fix BSL so an empty ignoreList doesn't mean ignore everything

#

if (ignores.length == 1 && ignores[0].isEmpty()) ignores = new String[0]; or something like that

tiny flare Nov 18, 2024, 6:34 PM

#

could do that too but I'll also remove the ignoreList entirely from server args since we don't need it anymore

#

this is getting out of hand

#

Exception in thread "main" java.lang.IllegalStateException: Module named cpw.mods.bootstraplauncher was already on the JVMs module path loaded from C:\Users\bruno\Downloads\test-neodev-server-5\libraries\cpw\mods\bootstraplauncher\2.0.2\bootstraplauncher-2.0.2.jar but class-path contains it at location libraries\cpw\mods\bootstraplauncher\2.0.2\bootstraplauncher-2.0.2.jar
        at [email protected]/cpw.mods.bootstraplauncher.BootstrapLauncher.run(BootstrapLauncher.java:139)        at [email protected]/cpw.mods.bootstraplauncher.BootstrapLauncher.main(BootstrapLauncher.java:69)

what now

tardy pebble Nov 18, 2024, 6:40 PM

#

so duplicated

#

can't be both on the module path and on the (legacy-)classpath

tiny flare Nov 18, 2024, 7:00 PM

#

it's intended that it works with a duplication with @west torrent 's recent changes

#

however, it fails to compare the paths when one is relative and the other is absolute 😅

#

goddamn I guess I'll add back the module path to the ignore list for now

#

I want to get this PR finished, we can clean these things up later

west torrent Nov 18, 2024, 7:59 PM

#

tiny flare however, it fails to compare the paths when one is relative and the other is abs...

ooops

#

Yeah well I suppose that is true, but removing ignoreList should be an early fix in BSL

tiny flare Nov 18, 2024, 8:07 PM

#

🤷 I just want this PR to work

#

we'll look at BSL after

tiny flare Nov 18, 2024, 10:49 PM

#

ok, server installer works again

west torrent Nov 18, 2024, 11:37 PM

#

early fix after the PR 😛

tiny flare Nov 18, 2024, 11:40 PM

#

yeah we're on the same page

supple maple Nov 19, 2024, 8:26 AM

#

Gradle says "thou shalt not referenceth otherth projects" yet here we are

#

i agree

west torrent Nov 19, 2024, 8:26 AM

#

I told Tech 😄

#

Can be solved but is moar work

tiny flare Nov 19, 2024, 8:36 AM

#

It's also not relevant until we fix cross-project dependencies for the mods {} block

west torrent Nov 19, 2024, 5:06 PM

#

weirdly enough configureEach seems to run too late hm

tiny flare Nov 19, 2024, 5:26 PM

#

Oh yeah the task import might happen quite early?

west torrent Nov 19, 2024, 5:39 PM

#

i am not sure how exactly we set the group tbh

#

well i will investigate further

stone wigeon Nov 20, 2024, 9:48 PM

#

@tiny flare Does this thing let you launch vanilla in debug mode again?

tiny flare Nov 20, 2024, 9:48 PM

#

not yet but that's one of my plans

#

it should be very little code (famous last words) - just generating a dummy userdev config with the run types

west torrent Nov 20, 2024, 9:51 PM

#

I also thought about that

#

Since we have a NeoDevBasePlugin it shouldn't be a lot of steps, you could theoretically just launch the cursed MCP entrypoint 😄

tiny flare Nov 20, 2024, 9:51 PM

#

nah just launch MC directly

#

it's just another call to NeoDevFacade in theory

#

(+ a task to generatae the dummy config)

west torrent Nov 20, 2024, 9:57 PM

#

BTW did you actually check the produced Gradle metadata? as in, with MDG userdev?

#

      - Variant 'runtimeElements' capability net.neoforged![neoforge](https://cdn.discordapp.com/emojis/1291265252922753064.webp?size=128 "neoforge")21.3.67-beta-features-moddevgradle declares a component for use during runtime:
          - Unmatched attributes:
              - Doesn't say anything about net.neoforged.distribution (required 'client')

#

hmm

tiny flare Nov 20, 2024, 10:11 PM

#

I did

#

But not after the most recent changes?

west torrent Nov 20, 2024, 10:28 PM

#

I see nothing wrong with the .module file generated by the PR

tiny flare Nov 21, 2024, 7:00 AM

#

I'll have a look

tiny flare Nov 21, 2024, 9:51 AM

#

@west torrent thoughts on using a wildcard version for the installer? can't say I'm a fan

#

3.0.+ sounds quite safe though

tiny flare Nov 21, 2024, 10:47 AM

#

I am confused by the compile classpath for junit in :tests

#

for some reason it's not pulling in the higher junit version that junit-fml defines

#

oh of course - junit-fml uses implementation

west torrent Nov 21, 2024, 10:49 AM

#

Wait why would we use a wildcard version for the installer?

tiny flare Nov 21, 2024, 10:55 AM

#

so that it auto updates™️ all branches

supple maple Nov 21, 2024, 12:09 PM

#

@tiny flare i'd make the http client use virtual threads, would be an interesting experiment, maybe it is faster thinkies

tiny flare Nov 21, 2024, 12:16 PM

#

it's already not the bottleneck 😉

#

(the bottleneck is changelog generation I think 😒)

#

I'm happy to experiment more once the PR is merged though 😁

west torrent Nov 21, 2024, 1:27 PM

#

yes, the changelog generation is ... slow. I mentioned that before

#

I'd have to delve deep into GU though

tiny flare Nov 21, 2024, 2:40 PM

#

@supple maple https://github.com/neoforged/NeoForge/commit/5d2a23511d00ce8d3d476648488a6768094d5ee7 look at this

GitHub

Add renovate comments for tool dependencies. Use + version for inst...

#

I prefer to use the renovate: prefix

#

is the syntax correct?

#

also, do you need the :artifact suffix?

supple maple Nov 21, 2024, 2:44 PM

#

that looks fine

tiny flare Nov 21, 2024, 5:29 PM

#

west torrent > - Variant 'runtimeElements' capability net.neoforged<:neoforge:12912...

this variant should not be published, what did you do?

west torrent Nov 21, 2024, 5:30 PM

#

I just includeBUild

tiny flare Nov 21, 2024, 5:31 PM

#

yeah don't do that then

#

includeBuild was never supported why would it randomly work now 😅

#

looks like includeBuild is pulling configurations that are not published

#

no time to chase that now in any case - the MDG MDK is still working afaict

west torrent Nov 21, 2024, 5:44 PM

#

Yes okay, I wanted to check with those FML changes 😄

tiny flare Nov 22, 2024, 10:01 AM

#

loading client-extra from the classpath means that it has to be on the classpath 😅

#

which works fine, but I need a new NeoDevFacade API to run tasks on project sync

west torrent Nov 22, 2024, 11:19 AM

#

Hm what up?

#

And explain

tiny flare Nov 22, 2024, 11:43 AM

#

FML supports two ways of locating MC resources:

file whose name contains client-extra on the LCP
jar containing assets/.mcassetsroot on the system CP

#

maty suggested that we go from the former to the latter

#

to do so I need to move the extra resources jar to the system CP

#

however in that case, there is nothing triggering the creating of the file on project sync anymore (whereas the LCP is written on project sync which forces creation of the file)

west torrent Nov 22, 2024, 11:47 AM

#

Hmmm, isn't that just part of createMinecraftArtifacts?

#

Which should run on sync regardless?

tiny flare Nov 22, 2024, 11:54 AM

#

nothing is making it run on sync besides the resources on the LCP

tiny flare Nov 22, 2024, 12:03 PM

#

west torrent Which should run on sync regardless?

we can make it run on sync manually

#

the normal workflow is to call setup which will run it

tiny flare Nov 22, 2024, 4:53 PM

#

@supple maple any blocker on the PR? if no please approve 😛

tiny flare Nov 22, 2024, 5:17 PM

#

thanks

west torrent Nov 22, 2024, 5:21 PM

#

Task groups still outstanding

#

well wrong

#

https://github.com/neoforged/ModDevGradle/pull/186

GitHub

Use distinct branding for tasks when they're being used as part of ...

#

Not tested yet, but you can take a look already

tiny flare Nov 22, 2024, 5:26 PM

#

I'll merge the PR first so we get some testing asap

#

I'll have a look at branding a little bit later 😉

west torrent Nov 22, 2024, 5:29 PM

#

Lucky you, the NeoDev facade doesn't even change 😅

craggy shadow Nov 22, 2024, 7:48 PM

#

ohno , need to rebase the spotless-be-gone PR it seems

#

But also 🎉 hooray, I love the setup

tiny flare Nov 22, 2024, 7:53 PM

#

have you looked deeper into it yet?

craggy shadow Nov 22, 2024, 7:54 PM

#

tiny flare have you looked deeper into it yet?

Deeper into what?

tiny flare Nov 22, 2024, 7:55 PM

#

the details of the setup

craggy shadow Nov 22, 2024, 7:56 PM

#

A bit here and there. Seems fairly sensible -- can't say everything is the way I'd implement it were I writing a gradle plugin but, like, it's an in-tree plugin for specifically neodev, it doesn't have to be reusable or particularly extensible or whatever, it has to work well for this purpose and be maintainable and from the read-through of it I did a few days back it seems to be

#

I do have a few minor questions here or there about why stuff was implemented the way it was (why does NeoDevConfigurations#resolvable exist? And similar random, small things) but for the most part it's small stuff. Haven't ran into anything that seems like a big worry yet -- and even if there is, the scope it affects is pretty small, since it's just neodev, which makes it easier to fix up stuff and iterate

#

One complaint about the state of neodev in general -- can we not have mavenLocal() be a thing by default? I feel like I keep seeing folks run into confusion when a local version of something is used instead of the proper published version, and I've ran into that myself, and all in all it means that the same environment can act differently on different machines; is there any use case for it that can't be solved by having it controlled by a gradle.properties file or something so folks can easily turn it on if they do want to use local stuff to test?

#

Also took a look through remaining TODOs noted in that commit -- the stuff in testframework, notably. The compileOnly project(path: ':neoforge', configuration: 'apiElements') dep will not leak to the pom; the runtimeOnly project(path: ':neoforge', configuration: 'runtimeElements') may. That said -- why do these specify a configuration? Shouldn't they just both be implementation project(':neoforge') to do what both of those do, combined?

#

Secondly, the source set compile/runtime classpath fuckery done in the same project -- there is, like, no world where you should ever need to do that sort of stuff, nor a world where you should actually do it, in practice, so -- something has gone very wrong if that is necessary for some reason

tiny flare Nov 22, 2024, 8:11 PM

#

why resolvable? the corresponding gradle API is incubating and returns a provider which is a bit annoying

#

can we not have mavenLocal() be a thing by default?
yeah we can remove it

#

the way dependencies are propagated from the neoforge project to testframework and tests is not very clean atm you are right

#

the reason it is how it is, is because it works and I didn't feel like trying to improve it yet blobsweat

#

you are correct that it's weird

west torrent Nov 22, 2024, 8:35 PM

#

craggy shadow Secondly, the source set compile/runtime classpath fuckery done in the same proj...

Some stuff was really left as-is to just replace the plugin without also reworking the project structure

#

I also think the cross-project task access should be replaced with configurations but hey, we can now much more reasily iterate on that

tiny flare Nov 22, 2024, 8:38 PM

#

I have no idea how NG did the cross-project things

#

most of this weird stuff is of my own making 😅

#

quick PR: https://github.com/neoforged/NeoForge/pull/1697

GitHub

Add Vanilla runs in NeoForge development environment by Technici4n ...

#

also works with Eclipse 🙂

craggy shadow Nov 22, 2024, 9:08 PM

#

west torrent I also think the cross-project task access should be replaced with configuration...

Exactly my thoughts. Yes, there's things I'd say could be improved, but its a lot easier to iterate now

craggy shadow Nov 22, 2024, 9:08 PM

#

tiny flare why `resolvable`? the corresponding gradle API is incubating and returns a provi...

Yeah but there's additional enforcement -- namely, that nothing changes the configuration later -- that the corresponding gradle API does. And you can just .get() the provider, it's safe

tiny flare Nov 22, 2024, 9:09 PM

#

Ah yes I see

#

We're not too disciplined with finalizeValueOnRead and similar things

craggy shadow Nov 22, 2024, 9:11 PM

#

Yeah... not a big deal, just general safety stuff that avoids footguns. finalizeValueOnRead wouldn't even help here as it's not a property -- the configuration created by gradle's API for explicitly resolvable-but-not-consumable configurations is actually a different type than normal configurations

tiny flare Nov 22, 2024, 9:21 PM

#

ah yeah that's how they do it

craggy shadow Nov 22, 2024, 11:29 PM

#

What's extra nice is that the type is actually exposed -- I have a suspicion at some point in the future all configurations will have to be made that way so you'll actually be able to do stuff a bit more reliably with regard to configurations and what they're actually doing

#

In other words, the separate concerns of outgoing and incoming stuff may actually be properly separated...

tiny flare Nov 22, 2024, 11:41 PM

#

ah yeah, I didn't even notice that

tardy pebble Nov 23, 2024, 9:23 AM

#

y'all don't forget that once all this has settled down, we're closing this thread and moving it to the main channels thinkies

tiny flare Nov 23, 2024, 9:24 AM

#

we can do that already 😉

tardy pebble Nov 23, 2024, 9:24 AM

#

oh good

#

which channel, then?

#

just so we're clear

supple maple Nov 23, 2024, 9:26 AM

#

#tooling-dev

tiny flare Nov 23, 2024, 9:29 AM

#

that or #neoforge-github

#Custom Gradle plugin for NeoForge itself