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

Port luaj to Gradle and improve build script #186

Open
wants to merge 23 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@dmarcuse
Contributor

dmarcuse commented May 4, 2017

In this PR I've ported LuaJ from ant to Gradle, and then added it as a subproject dependency for the main project (CC itself). This means that 1) dedicated deploy and build_luaj script are no longer needed, you can simply run ./gradlew build to get a functional jar (which also contains luaj automatically). This also makes it much easier for people trying to use CC as a dependency in their own projects, because LuaJ is now properly registered as a dependency and included in the output jar.

I'm happy to make changes if necessary, just leave a comment if there's anything I should do.

distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-2.7-bin.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-3.5-bin.zip

This comment has been minimized.

@JLLeitschuh

JLLeitschuh May 4, 2017

Contributor

Yeay!

@@ -0,0 +1,172 @@
#!/usr/bin/env sh

This comment has been minimized.

@JLLeitschuh

JLLeitschuh May 4, 2017

Contributor

You don't need the gradle wrapper in the luaj subproject.
You can just use the parent gradlew wapper to run the project.

This comment has been minimized.

@dmarcuse

dmarcuse May 4, 2017

Contributor

Thanks for pointing that out, fixed.

@SquidDev

This comment has been minimized.

Contributor

SquidDev commented May 4, 2017

I wonder if it would be worth adding CurseGradle support? This means you can upload to CurseForge with a single Gradle command (./gradlew build curseforge). Something like:

plugins {
    id 'com.matthewprenger.cursegradle' version '1.0.8'
}
curseforge {
	apiKey = project.hasProperty('curseForgeApiKey') ? project.curseForgeApiKey : ''

	project {
		id = '67504'
		releaseType = 'release'
		changelog = ''
	}
}

You can then define a Curse API key in ~/.gradle/gradle.properties and it all "just works". I think it would be worth checking with Dan before adding this though.

@dmarcuse

This comment has been minimized.

Contributor

dmarcuse commented May 4, 2017

I like the idea of CurseGradle, but as it's something that would only be used by Dan it should be up to him as you said.

@JLLeitschuh

This comment has been minimized.

Contributor

JLLeitschuh commented May 4, 2017

The curse key could be encrypted and embedded in travis environment for auto-releases there. Again, something dan would need to do.

@@ -83,3 +76,6 @@ processResources
}
}
jar {
from configurations.shade.collect { it.isDirectory() ? it : zipTree(it) }
}

This comment has been minimized.

@JLLeitschuh

JLLeitschuh May 4, 2017

Contributor

I think to keep the configuration phase fast for gradle I think you want this instead:

jar {
     from  { configurations.shade.collect { it.isDirectory() ? it : zipTree(it) } }
    dependsOn configurations.shade
 }

Double check with that code to make sure I'm right.

The way you have the code defined now, the file and all of its dependences have to be resolved at configuration time instead of doing it at task execution time.

This comment has been minimized.

@SquidDev

SquidDev May 4, 2017

Contributor

Alternatively, you might want to do:

jar {
    configurations.shade.each {
        from(it.isDirectory() ? it : zipTree(it)) { exclude 'META-INF', 'META-INF/**' }
    }
}

That way you don't include any of the META-INF stuff. I don't think it matters, but it might be safer.

@dan200

This comment has been minimized.

Owner

dan200 commented May 4, 2017

This is a gargantuan commit just to avoid having to type "./build_luaj.sh" on the once in a blue moon you make a change to the luaj sources, no? Is the objection because that script doesn't work on windows? Because I'm sure we could make a .bat file easilly.

setup.sh Outdated
@@ -1,14 +0,0 @@
#!/bin/sh

This comment has been minimized.

@dan200

dan200 May 4, 2017

Owner

Why are you deleting perfectly useful scripts?

@dmarcuse

This comment has been minimized.

Contributor

dmarcuse commented May 4, 2017

The point of this PR is two things mostly:
1 - no more need for separate scripts to build things, you can do it all from gradle easily which eliminates compatibility issues (e.g. needing to install unzip package for the deploy script to work)
2 - adding luaj to the dependencies of the project properly (rather than /lib/) and packaging it into the gradle output jar allows it to work properly when CC itself is used as a dependency. For example, while attempting to port CCEmuX to use the new open source repository, it was impossible to get it working because the gradle artifacts neither contained luaj nor required luaj as a dependency, so the classes were not present.

Additionally, there are various buildscript improvements such as setting the target and source compatibility to ensure that everything works with old Java versions, and adding better support for Eclipse.

I got a bit delete-happy with the scripts, I'll add them back - setup.sh, setup.bat only, or any of the others as well?

@dan200

This comment has been minimized.

Owner

dan200 commented May 4, 2017

I would keep them all except build_luaj.sh, even if all they do is call through to the gradlew

@dmarcuse

This comment has been minimized.

Contributor

dmarcuse commented May 4, 2017

Would you be opposed to me rewriting the scripts as Gradle tasks? This eliminates compatibility issues and makes them work properly across operating systems - e.g. ./gradlew deploy, ./gradlew ideSetup, etc.

If you like, I can still add scripts that simply call these tasks, this would just make it easier to maintain compatibility.

group = 'build'
description = 'Generates a jar and a source jar for release'
dependsOn jar

This comment has been minimized.

@SquidDev

SquidDev May 5, 2017

Contributor

This needs to depend on assemble instead, otherwise the retromapReplacedMain task will not run, meaning the resulting file will not be obfuscated.

This comment has been minimized.

@dmarcuse

dmarcuse May 5, 2017

Contributor

TBH I'm not sure why we need a separate task for deploy since you can just run build to get the release and source jars (which now contain LuaJ and such)

@dmarcuse

This comment has been minimized.

Contributor

dmarcuse commented May 5, 2017

@dan200 I've added back the functionality from the scripts (as Gradle tasks) and added wrappers for the tasks, could you review this again and let me know if anything else should be changed?

@SquidDev

This comment has been minimized.

Contributor

SquidDev commented May 5, 2017

From what I can tell this doesn't package the API source or documentation in the JAR. I think you can do it like this:

javadoc {
    include "dan200/computercraft/api/**/*.java"
}

jar {
    dependsOn javadoc
    into("docs", {  from(javadoc.destinationDir) })
    into("api", { from (sourceSets.main) {
        include "dan200/computercraft/api/**/*.java"
    }})
}

dmarcuse added some commits May 5, 2017

Add sources to output jar
Not adding javadoc yet because LuaJ spits a lot of errors and causes the
javadoc task to fail, and most IDEs can pull docs directly from source
anyways
@SquidDev

This comment has been minimized.

Contributor

SquidDev commented May 10, 2017

I'd really like to see this merged, or at least a trimmed down version of it. There have been several people on IRC, various discords and the forums wondering why ComputerCraft "doesn't work", not realising they need to use the ./deploy.sh script - and even then having issues. Doing the bundling inside the Gradle task would be much less confusing for everyone.

@KnightMiner

This comment has been minimized.

KnightMiner commented May 16, 2017

I also would like to see this get merged. I currently have to manually copy the Lua dependency into my mods folder (Forge classpath injection feature) since the scripts do not seem to function fully on Windows.

LICENSE Outdated
@@ -36,7 +36,7 @@ This mod is provided 'as is' with no warranties, implied or otherwise. The owner
of this mod takes no responsibility for any damages incurred from the use of
this mod. This mod alters fundamental parts of the Minecraft game, parts of
Minecraft may not work with this mod installed. All damages caused from the use
or misuse of this mad fall on the user.
or misuse of this mod fall on the user.

This comment has been minimized.

@lupus590

lupus590 Jul 30, 2017

Contributor

Just highlightings this as it is a good change but doesn't fit the rest of the pull request

This comment has been minimized.

@Lignum

Lignum Jul 30, 2017

Contributor

This has already been fixed in #337.

@lupus590

This comment has been minimized.

Contributor

lupus590 commented Jul 30, 2017

I'm not an expert on Java or Gradle yet, but there seems to be a lot of noise/unrelated changes in that last commit.

@Lignum

This comment has been minimized.

Contributor

Lignum commented Jul 30, 2017

@lupus590 It's because it's a merge commit, all those changes are just catching up. They shouldn't appear in the "Files changed" tab

@SquidDev

This comment has been minimized.

Contributor

SquidDev commented Jul 30, 2017

@lupus590 He merged several hundred commits, so there is going to be a little bit of noise :).

@apemanzilla I wonder if it would be better to rebase the PR and squash everything so it's a little clearer what's changed. It might be worth reverting the LuaJ layout change, and just getting gradle to point to different directories - that way the change list is a little less murky.

@KnightMiner

This comment has been minimized.

KnightMiner commented Jul 30, 2017

Might be good to rebase this just to prevent odd errors rather than the merge commit, I've seen issues from merge commits with conflicts before.

@dmarcuse

This comment has been minimized.

Contributor

dmarcuse commented Jul 30, 2017

Yeah, I should have rebased instead of merging, didn't realize it was that many commits behind...

Either way, as stated before, if you use the Files Changed button at the top you'll see the actual changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment