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

feat: load modules from classpath (for dependencies) #41

Closed
wants to merge 5 commits into from

Conversation

keturn
Copy link
Contributor

@keturn keturn commented Feb 26, 2021

If engine's ModuleManager doesn't load all modules on the classpath by default, how are MTE tests going to get their dependencies?

Companion to MovingBlocks/Terasology#4543

@keturn
Copy link
Contributor Author

keturn commented Feb 26, 2021

Something along these lines?

except this doesn't actually work, as all the tests I've tried so far fail in a fiery death of NullPointerExceptions, but they do get past the dependency resolution stage.

@keturn
Copy link
Contributor Author

keturn commented Feb 26, 2021

current status: lots of things passing in IntelliJ but failing under gradle.

To provide a little more information when things fail and end up with NPEs later.
@keturn
Copy link
Contributor Author

keturn commented Feb 28, 2021

I think I'm starting to see what's happening with the mess I'm in.

The errors I'm running in to are when environment.getModuleProviding comes up empty: https://github.com/MovingBlocks/gestalt/blob/d8b35c056c5663d73497acdda7c01b9e32b0b439/gestalt-module/src/main/java/org/terasology/module/ModuleEnvironment.java#L298-L303

Asked about a type, it looks at that type's location, and then looks through all the environment's modules to see if any of them were registered with that location.

When we only have jars for everything, this is fine.

When we have sources and we're running under IntelliJ, it's also fine, because it puts modules/**/classes/ on the classpath and the modules are registered that way.

Problem comes up when we have sources and we run under gradle. Gradle puts jars on the classpath, but when ModuleManager scans the modules/ path, we end up with the classes directories. Even if we don't load register the jars on the classpath as modules, the being passed to getModuleProviding still come from those jars.

So.

Options:

  • 🐘 teach gradle that we prefer our runtime dependencies as classes directories instead of jars.
  • 🕶 have MTE only load modules on classpath and do not add anything from modules/ at all.
  • ♞ allow the ModuleManager to load its normal modules paths, but if there is ever any conflict when we add things from classpath, then make sure the classpath's version is the one that ends up in the environment.
  • ♘ allow the ModuleManager to load its normal modules paths, but change some classloading stuff somewhere so the only references the MTE engine has to those types are the ones from our modules, not the system classpath.

🐘 I think it's possible do that thing with gradle, but there's no built-in option for it (at least as of gradle 6.8), so it'd be significant work.

🕶 preventing MTE from scanning modules/ is probably what makes the most sense. This is already configurable in PathManager, more or less. I doubt we can get away with setting the list of module paths as empty, because this thing that expects there to always be one https://github.com/MovingBlocks/Terasology/blob/220c52dcb9d5a7b68f9d23bff2f80c85daf94649/engine/src/main/java/org/terasology/engine/paths/PathManager.java#L349 but we can set that to be in MTE's temp directory.

@keturn
Copy link
Contributor Author

keturn commented Mar 2, 2021

Time for another Terasology Build Tune-Up Tuesday, with more fun facts about Gradle and IntelliJ!

As we covered earlier, gradle puts jars on the runtime classpath, not classes directories.

Except for the current subproject under test. The difference is probably because main and test are source sets within the same project, not a dependency between projects.

This becomes especially relevant when the build has things that are only triggered when building the jar, as is the case here: https://github.com/MovingBlocks/Terasology/blob/220c52dcb9d5a7b68f9d23bff2f80c85daf94649/buildSrc/src/main/kotlin/terasology-module.gradle.kts#L192-L197

That's one reason why your tests might not be finding module resources when invoked by gradle.

As for IntelliJ IDEA:

When IntelliJ IDEA imports a gradle project, it makes an IdeaModule (not to be confused with a Terasology module!) for the project, and sub-modules for the main and test source sets.

  • 📂 Foo - Foo/
    • 📁 Foo.main - Foo/src/main/
    • 📁 Foo.test - Foo/src/test/

The complication for us comes in when we consider resources. IntelliJ IDEA won't run a gradle processResources task by default, but it will at least pick up a few things from the source set.

The assets of a Terasology module are stored in Foo/assets.

We can add Foo/assets as a resource directory to Foo.main. That seems to work, but a file assets/textures/foo.png ends up under the resource path textures/foo.png, not the intended assets/textures/foo.png. Its resource name is relative to the resource directory; the name of the resource directory itself is not included.

If we were using IntelliJ alone, we would solve this by setting the relative output path for assets as a resource root. But IntelliJ doesn't use gradle's output paths, so it doesn't pick up on that from the SourceSet's output properties.

In gradle, we can set Foo/ as the source with an include pattern to match assets (and deltas and overrides) without everything else. But that's too sophisticated for the gradle-IntelliJ translation. When IntelliJ sees that Foo.main is trying to claim Foo/ as a content root, it cries foul, complaining that Foo/ is already taken as a content root by its parent IdeaModule.

Long story short: Avoid build configuration headaches by keeping your resources in a resources directory.

@keturn
Copy link
Contributor Author

keturn commented Mar 2, 2021

In recent comments on #4543, I suggest giving up the current attempt to move ModuleLoadingStuff over here.

If we do that, does anything in this PR remain relevant?

It looks like there are some debugging nice-to-haves we can extract to a separate PR, and might as well start a fresh one for whatever toggle mechanism #4543 ends up offering us.

"Should we care if classpath modules are also on the module path?" is a question worth remembering,

Stream<Path> pathsOutsideModulesDirectory = allClassPaths.filter(classPath -> pathManager.getModulePaths().stream().anyMatch(
other -> !classPath.startsWith(other)
));

but we can probably get away with not answering it until after gestalt-v7.

@keturn
Copy link
Contributor Author

keturn commented Mar 3, 2021

extracted some things to #42.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant