Updates to the new build system #210

ThadHouse · 2017-08-01T17:47:14Z

Jenkins will fail until I update the scripts there, which I will do in a bit.

JLLeitschuh · 2017-08-02T03:06:21Z

build.gradle

+        binaries {
+            withType(SharedLibraryBinarySpec) { binary ->
+                if (binary.component.name == 'ntcoreJNI') {
+                    from(binary.sharedLibraryFile) {


Doesn't this cause this to get resolved eagerly at configuration time? Don't you want this to get resolved at task runtime?

It does not. The native builds handle eagerly configuring this. It does get resolved at runtime.

JLLeitschuh · 2017-08-02T03:07:13Z

config.gradle

+
+def buildAll = project.hasProperty('buildAll')
+
+def windows64PlatformDetect = { config->


Why does this closure take an arg but never use it?

I always forget in gradle you can ignore closure arguments. There are other cases where that config is used, but I can remove it from here.

JLLeitschuh · 2017-08-02T03:09:13Z

publish.gradle

-    project(':arm:wpiutil').build.dependsOn outputVersions
-    project(':arm:ntcore').build.dependsOn outputVersions
-}
+def licenseFile = new File("$rootDir/license.txt")


Could just use file("$rootDir/license.txt")

JLLeitschuh · 2017-08-02T03:10:19Z

publish.gradle

+    model {
+        tasks {
+            it.each {
+                if (it.name.contains('jniHeaders')) {


You might also want to check that the type is a CopySpec?

Yeah probably.

Actually the task it is looking at is not a copy task, so that doesn't work. This is one of the few cases where it is just hard to get something out of the model.

What type is it then?

https://github.com/wpilibsuite/native-utils/blob/master/src/main/groovy/edu/wpi/first/nativeutils/rules/JNIConfigRules.groovy#L40
It's just a Task, it doesn't inherit from anything.

Reading through that code, just as an aside, its much better practice to create types for your tasks. Doing so allows you to use the annotations @Input, @InputDirectory, @Output, @OutputDirectory ect... That allow your build to do things better with task avoidance (speeding up the build).
Additionally, doing so would allow you to take advantage of the new gradle build cache where you can share the output of tasks on a central server (or even on your personal machine) to avoid running tasks that you've already run before.

Additionally, I worry that the lack of any sort of type system here will make this code unmaintainable by anyone in the future. Without a solid set of tests to prove that what the build is creating is correct it's much harder for new developers to dive in without the fear of breaking something.

How are the annotations different from just declaring Inputs and Outputs on the task, which I already do. Also, until all the native stuff supports the build cache (which none of it does currently), I don't think its necessary to take the extra work. I do agree that adding a custom type for this might be a good idea however. I just wish the team had commented on this weeks ago when I asked for help and suggestions rather then waiting until we were wanting to merge it.

@JLLeitschuh I created custom types for my tasks. I didn't add anything to them, since the default task worked just fine, but it does let me do type checking.

JLLeitschuh · 2017-08-02T03:12:51Z

publish.gradle

-            if (project.hasProperty('makeDesktop')) {
-                artifact nat.ntcoreZip, {
-                    classifier = 'desktop'
+def createComponentZipTasks = { components, name, base, type, project, func ->


Maybe turn this into a more strongly typed groovy function instead of using a closure since there are so many inputs? It would help others read it.

Good idea. Something similar to this is used in most of the repo's, so thats a good idea.

I'm going to pass on this, because I use a few script globals, and switching to a function breaks that, with the workarounds looking super hacky

JLLeitschuh · 2017-08-02T03:14:09Z

src/test/java/TestClass.java

+public class TestClass {
+    @Test
+    public void generalTestCase() {
+        assertEquals(true, true);


What is this testing? Perhaps a bit more documentation if you are actually expecting something here.

This was more to verify that tests were actually running, and the test below is to make sure the JNI linking actually works. I'll remove the general one, and move the JNI one to it's own package.

JLLeitschuh · 2017-08-02T03:15:03Z

src/test/java/TestClass.java

@@ -0,0 +1,17 @@
+import org.junit.BeforeClass;


This seems to be in the default package. This is not really done even in test systems. You should put this test in its own package.

Ok I will move it.

PeterJohnson · 2017-08-03T03:55:23Z

src/main/java/edu/wpi/first/wpilibj/networktables/NetworkTablesJNI.java

-  public static final int NT_NET_MODE_CLIENT = 0x02;
-  public static final int NT_NET_MODE_STARTING = 0x04;
-  public static final int NT_NET_MODE_FAILURE = 0x08;
-


This seems like an unintentional revert. Are you synced up to the latest master?

PeterJohnson · 2017-08-03T03:55:37Z

src/main/java/edu/wpi/first/wpilibj/networktables/NetworkTablesJNI.java

@@ -154,7 +136,6 @@ else if (System.getProperty("os.name").startsWith("Mac"))
  // public static native byte[] getRpcResultNonblocking(int callUid) throws RpcNoResponseException;

  public static native void setNetworkIdentity(String name);
-  public static native int getNetworkMode();


PeterJohnson · 2017-08-03T13:01:46Z

publish.gradle


 task outputVersions() {
-    description = 'Prints the versions of ntcore and wpiutil to a file for use by the downstream packaging project'
+    description = 'Prints the versions of wpiutil to a file for use by the downstream packaging project'


s/wpiutil/ntcore/

AustinShalit · 2017-08-03T14:53:02Z

build.gradle


 apply from: 'publish.gradle'

 task wrapper(type: Wrapper) {
-    gradleVersion = '3.3'
+    gradleVersion = '4.0.1'


Should we bump to 4.0.2?

JLLeitschuh · 2017-08-03T16:32:19Z

publish.gradle

+            it.each {
+                if (it.name.contains('jniHeaders')) {
+                    from (it.outputs.files) {
+                        into '/'
                    }


When does this execute? It looks like it runs during task configuration time, not at task run time? Is that what you want?

This does execute at run time. Otherwise copy and zip tasks would be absolutely useless.

Can you explain how that's possible? It seems like the receiver of the call to from is project not the task. As you said above, the task is of type DefaultTask which doesn't have a from method on it.

If the receiver of the from call is project then it would be executed immediately.

What is the receiver of the from call?

The cppSourcesZip. GitHub is just splitting the lines wierdly.

Oooooh. That makes so much more sense.

Fixes mac builds Updates plugin to fix mac builds Removes unnecessary closure variable Plugin 1.2.4 Updates plugin to 1.2.5 Uses plugin .6 plugin 1.7 Plugin 1.2.8 Fixes review issues

removes wpiutil

740d3a6

ThadHouse force-pushed the newBuild branch 10 times, most recently from 73772e5 to f7f3cd2 Compare August 1, 2017 20:49

JLLeitschuh reviewed Aug 2, 2017

View reviewed changes

ThadHouse force-pushed the newBuild branch 3 times, most recently from 67288bf to d2b3077 Compare August 3, 2017 02:48

PeterJohnson suggested changes Aug 3, 2017

View reviewed changes

ThadHouse force-pushed the newBuild branch from d2b3077 to b062a23 Compare August 3, 2017 03:59

PeterJohnson suggested changes Aug 3, 2017

View reviewed changes

AustinShalit reviewed Aug 3, 2017

View reviewed changes

ThadHouse force-pushed the newBuild branch 2 times, most recently from ac7409f to d2fe992 Compare August 3, 2017 15:45

JLLeitschuh reviewed Aug 3, 2017

View reviewed changes

updates to the new build system

a7fbd7e

Fixes mac builds Updates plugin to fix mac builds Removes unnecessary closure variable Plugin 1.2.4 Updates plugin to 1.2.5 Uses plugin .6 plugin 1.7 Plugin 1.2.8 Fixes review issues

ThadHouse force-pushed the newBuild branch from d2fe992 to a7fbd7e Compare August 3, 2017 18:56

PeterJohnson approved these changes Aug 3, 2017

View reviewed changes

PeterJohnson merged commit 5df7463 into wpilibsuite:master Aug 3, 2017

This was referenced Aug 13, 2017

Cannot build without x86 headers on linux? #43

Closed

Gradle builds don't detect 32 bit only devices #49

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to the new build system #210

Updates to the new build system #210

ThadHouse commented Aug 1, 2017

JLLeitschuh Aug 2, 2017

ThadHouse Aug 2, 2017

JLLeitschuh Aug 2, 2017

ThadHouse Aug 2, 2017

JLLeitschuh Aug 2, 2017

JLLeitschuh Aug 2, 2017

ThadHouse Aug 2, 2017

ThadHouse Aug 2, 2017

JLLeitschuh Aug 3, 2017

ThadHouse Aug 3, 2017

JLLeitschuh Aug 3, 2017

ThadHouse Aug 3, 2017

ThadHouse Aug 3, 2017

JLLeitschuh Aug 2, 2017

ThadHouse Aug 2, 2017

ThadHouse Aug 2, 2017

JLLeitschuh Aug 2, 2017

ThadHouse Aug 2, 2017

JLLeitschuh Aug 2, 2017

ThadHouse Aug 2, 2017

PeterJohnson Aug 3, 2017

PeterJohnson Aug 3, 2017

PeterJohnson Aug 3, 2017

ThadHouse Aug 3, 2017

AustinShalit Aug 3, 2017

JLLeitschuh Aug 3, 2017

ThadHouse Aug 3, 2017

JLLeitschuh Aug 3, 2017

ThadHouse Aug 3, 2017

JLLeitschuh Aug 3, 2017

ThadHouse Aug 3, 2017

JLLeitschuh Aug 3, 2017


		def buildAll = project.hasProperty('buildAll')

		def windows64PlatformDetect = { config->

Updates to the new build system #210

Updates to the new build system #210

Conversation

ThadHouse commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment