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

Download runtime library dependencies, resolve conflicts. #889

Merged
merged 2 commits into from Apr 27, 2018

Conversation

Projects
None yet
5 participants
@smartboyathome
Contributor

smartboyathome commented Mar 28, 2018

Adapting the changes of #865 after #879 was merged in.

This PR adds the ability for Glowstone to download dependencies for libraries specified in the glowstone.yml. It does so using an abstraction on top of Maven's Aether API, Naether, that makes dependency resolution fairly easy. I check the tree that is built up by this plugin against the libraries builtin to the server by searching the classpath for the dependency's POM, which maven-shade-plugin embeds into the JAR. I tried various methods (hence why this change took so long), and this seems to be the most stable way of attempting to do this.

Now, there's a few downsides to how I implemented this:

  • The way I am checking for what dependencies Glowstone includes relies on an implementation detail from maven-shade-plugin. While not a large piece of code, it will have to be rewritten if we ever decide to move away from this plugin or from Maven in general. I tried exploring other possibilities, such as serializing the depndency tree at build time, but it proved to be a lot more work for not much more benefit.
  • This requires embedding portions of Maven itself into Glowstone in order to perform the dependency resolution. While this doesn't tie us to Maven for builds like the dependency checking does, it still ties us to resolving dependencies in a certain way. If we decide to move build systems, it could be awkward to have two different ways of resolving dependencies depending on if they are runtime vs builtin. Currently, we're using Maven, so I think it's fine to use it for our library downloads as well.
  • Naether provides the ability to download the dependencies itself, but I am explicitly disabling that since we have our own download manager. We may choose to use this at some point in the future, but I felt it was better to let this functionality bake before trying this out.
  • Checksums are required for publishing on Maven Central, so we could take advantage of those in many cases, but I decided to put that as a possible future task rather than making this change even bigger.

Do note that due to concerns in the previous commit, I reverted my Kotlin changes. Kotlin will still be included with the Glowstone JAR file.

As before, let me know what you think of this change! I think the benefits of this outweigh the downsides listed above, but certainly there's room for improvement here.

@Pr0methean

This comment has been minimized.

Contributor

Pr0methean commented Mar 30, 2018

Will this affect the ability to use libraries that aren't on Maven Central?

* dependencies.
*/
private static final Set<LibraryKey> blacklistedRuntimeLibs = ImmutableSet.of(
new LibraryKey("it.unimi.dsi", "fastutil")

This comment has been minimized.

@kashike

kashike Mar 31, 2018

Why is this blacklisted? What problems does it cause?

This comment has been minimized.

@smartboyathome

smartboyathome Mar 31, 2018

Contributor

It causes classpath conflicts with FastUtil-Lite, which we include in the server JAR.

@smartboyathome smartboyathome force-pushed the smartboyathome:lib-dep-downloads branch from cd970ca to 2eceb05 Apr 15, 2018

@smartboyathome

This comment has been minimized.

Contributor

smartboyathome commented Apr 15, 2018

After a while, finally got back to this. I made a small change to better address using libraries that aren't in a maven repository. Prior to the most recent commit, using a library not in a maven repo would need to use an invalid URL as the repository key in order to not affect any other dependencies being resolved. Now, you can set the exclude-dependencies flag within the library's config section to true in order to keep from attempting to resolve the dependencies for these libraries.

@mastercoms

This comment has been minimized.

Member

mastercoms commented Apr 25, 2018

@smartboyathome Could you please resolve the merge conflicts?

@smartboyathome

This comment has been minimized.

Contributor

smartboyathome commented Apr 25, 2018

Sure, I can do that later today.

@smartboyathome smartboyathome force-pushed the smartboyathome:lib-dep-downloads branch from 2eceb05 to 01d51ba Apr 27, 2018

@smartboyathome

This comment has been minimized.

Contributor

smartboyathome commented Apr 27, 2018

Merge conflicts resolved.

@mastercoms mastercoms merged commit 610bdcb into GlowstoneMC:dev Apr 27, 2018

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment