Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Runtime mod remapping in dev #241

Merged
merged 5 commits into from
Sep 26, 2020

Conversation

modmuss50
Copy link
Member

@modmuss50 modmuss50 commented May 13, 2020

I tried to cause as little impact to existing parts of the code as possible, this may have lead to things not being ideal.

FabricMC/fabric-loom#277 is required to export the intermediary classpath for this to remap mods against.

@modmuss50
Copy link
Member Author

There are some issues with this, that might need some more work from loom's side of things.

The mod deps on the classpath are not going to be in the right namespace to handle the remapping of things correctly. I think loom is going to need to pass mc and all of the deps as intermediary jars to be used for remapping.

@Pyrofab
Copy link
Contributor

Pyrofab commented May 15, 2020

Is this actually needed anyway ? We can easily flatDir a mods folder in our own buildscripts already, and that lets us see the source. Will this PR let us debug more easily ?

@modmuss50
Copy link
Member Author

this is not for depending on mods, this is purely for testing with mods.

Using flatdir enables you to depend and code against another mod, in a lot of cases this isnt needed. Being able to drop the mods into the mods folder is great when you need to test something quick or just want REI or some simple mod.

@Pyrofab
Copy link
Contributor

Pyrofab commented May 15, 2020

The issue is that when you want to test something quick, you often want to get the debugging tools out quickly in case of problems arising. I think something more useful would be a dependency type that is not transitive at all (not declared in maven).

@Juuxel
Copy link
Member

Juuxel commented May 15, 2020

a dependency type that is not transitive at all

That's modRuntime (or modCompileOnly for compile time deps).

@Pyrofab
Copy link
Contributor

Pyrofab commented May 15, 2020

Those are not transitive ? Well then, is there anything we need other than modRuntime fileTree(dir: 'mods, include: '*.jar') ?

@comp500
Copy link

comp500 commented Jun 2, 2020

I'll note (as was mentioned in Discord earlier) that it may be useful to support jar-in-jars, especially for modpacks. Not critical though, as there are tools to remove them.

…me_mod_remapping

# Conflicts:
#	src/main/java/net/fabricmc/loader/FabricLoader.java
modmuss50 added a commit to modmuss50/fabric-loom-1 that referenced this pull request Sep 13, 2020
@modmuss50 modmuss50 marked this pull request as ready for review September 13, 2020 14:33
@modmuss50 modmuss50 changed the title First pass of runtime mod remapping in dev Runtime mod remapping in dev Sep 13, 2020
@modmuss50
Copy link
Member Author

This is now ready for review, I have a little more testing to do with it. But so far it seems to be working well.

@@ -151,6 +154,100 @@ public void read(BufferedReader reader, String currentNamespace) throws IOExcept
classes.addAll(parentClasses);
}

public void write(StringWriter writer) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be worth moving access widerners out into their own lib, as all this is copyied right out of loom.


private final FabricLauncher launcher = FabricLauncherBase.getLauncher();

public Collection<ModCandidate> remap(Collection<ModCandidate> modCandidates, FileSystem fileSystem) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't all the methods in this class effectively be static?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it. Yes.

modmuss50 added a commit to FabricMC/fabric-loom that referenced this pull request Sep 26, 2020
* Export rumtime remap classpath for FabricMC/fabric-loader#241

* Fix bad merge
@modmuss50 modmuss50 self-assigned this Sep 26, 2020
@modmuss50 modmuss50 merged commit 69f3edf into FabricMC:master Sep 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants