Summer 2017 #22

AdamBJ · 2017-08-24T20:13:53Z

This PR improves upon the README, fixes grammatical errors in comments, fixes a bug in UmpleGradlePlugin.groovy that was causing an IllegalArugmentException to be thrown when relativize was called, and includes bug fixes relating to the integrationTest task (we should now once again be able to make the integration tests a normal part of the build process).

This branch isn't quite ready to be merged yet, have a look at line 27 of TestProject's build.gradle file (src/integrationTest/resources/TestProject/build.gradle). For some reason gradle thinks that the outputs of the compileUmple task are up-to-date all the time, even if the output files have been deleted. I was only able to get it working by apply the band aid you see starting at line 27. I was looking here to try and figure out what the problem was, but no luck yet. I think the solution may be annotating UmpleGenerateTask differently?

Improve README, fix grammatical errors in comments, fixed bug in UmpleGradlePlugin.groovy that was causing an IllegalArugmentException to be thrown when relativize was called, bug fixes relating to the integrationTest task.

AdamBJ · 2017-08-24T20:14:33Z

src/integrationTest/resources/TestProject/build.gradle

-
+
+// TODO this should not be neccesary. Find out why compileUmple is returning "UP-TO-DATE" when the output files have been deleted. 
+compileUmple {


We should get rid of this work around before merging this PR.

This TODO should be fixed by using IncrementalTaskInputs. You can see how I used them here: https://github.com/assertj/assertj-generator-gradle-plugin/blob/9f0ec19e910a3d7a4add5de461481454688063d8/src/main/groovy/org/assertj/generator/gradle/tasks/AssertJGenerationTask.groovy#L67

I'm not sure if IncrementalTaskInputs are the answer. The problem isn't that gradle is mistaking a modified/removed input file for an unmodified input file, the problem is that gradle isn't checking whether generated/output files (i.e. java files) exist when it does it's "up to date" check for the compileUmple task. We really just need gradle to check to see if the output files have been removed, and if they have, to rerun the generation task.

That's what IncrementalTaskInputs does. If you register all of the produced files with ITI, it will remove/update them when required.

- Made the `compileUmple` task be finalized by the `compileJava` task. This allows the user to run `compileUmple` to generate java files and compile them (so long as the `compileGenerated` flag is set). Before, `compileJava` depended on `compileUmple`, which meant that `compileJava` needed to be executed in order for generation and compilation to occur. This changes also makes the integrationTest task a bit simplier.

AdamBJ · 2017-08-25T18:13:57Z

I made another commit today, small but important bug fix for the way generation followed by compilation works.

AdamBJ · 2017-08-26T16:43:42Z

src/main/groovy/cruise/umple/tasks/UmpleGenerateTask.groovy

 /**
 * Created by kevin on 15/03/2017.
 */
 class UmpleGenerateTask extends SourceTask {


@Nava2 The incremental build stuff appears to be working now. I think this branch can be merged into ss-config-n2, now?

I don't think that this is properly implemented. I think Gradle needs to know all files that are generated. We either need to keep track of the files created or figure out a way to have Umple tell us what files it will change/create. The intention of the incremental updates is that gradle can choose whether or not to generate new files and also what files to delete.

I think Gradle needs to know all files that are generated

Doesn't the outputDirs member variable tell it that?

As far as I can tell, other than the "nice to have" refactoring of integrationTest, this is the only real issue that needs to be resolved before merging this branch. What do you think of my earlier comment? I agree the current implementation looks a bit funky, but by setting up the outputDirs member variable like that, doesn't Gradle know where all the generated files will be?

No because it needs to know the files directly. Thats what the incremental input files parameter does.

Looking at the example here: https://docs.gradle.org/3.1/javadoc/org/gradle/api/tasks/incremental/IncrementalTaskInputs.html

What if we have an inputDirs member variable that contains the locations of each of the master files in addition to the outputDirs member variable that we have now? Then we'd basically just copy the if (!inputs.incremental) and inputs.outOfDate... stuff from the example... and hopefully we'd be on our way.

Yes, somewhat. That assumes that we want to always change all files. I don't think we have another choice. The way the umple compiler works requires us to regenerate the entire tree no matter what file changes.

So yeah, that should be the best we can do, but not the ideal case. 🙂

Getting the incremental build stuff working for the integration tests is proving tricky. It works after I rebuild the plugin, but if I don't rebuild it the generation phase gets skipped while the verification step runs anyway (!!?!?!?!), which causes the test to fail. Even better is that the printing to standard out stuff seems to largely be broken for the integration tests, so I'm more or less doing everything blind. It might have something to do with the fact that we're using the test-sets plugin for integrationTest, but I'm really not sure. It looks like I might be able to use GenerationTests.groovy as a template if I can figure how to hook everything together.

On an unrelated note, I'll be starting my final semester on Tuesday. I'm not sure how much time I'll have to contribute to this project over the semester, but I'll do my best to pop in every now and again.

The only thing I can think is to try and convert integrationTest into a "normal" test (using Project Runner). I'm not sure how to go about doing that, though. How are the current tests (CheckGlobals.groovy etc) hooked into the test task?

AdamBJ · 2017-08-26T16:45:24Z

src/main/groovy/cruise/umple/tasks/UmpleGenerateTask.groovy

+    void setOutputDirs(UmpleOptions newOpts) {
+        this.outputDirs = new ArrayList<File>()
+
+        UmpleOptions opts = newOpts


I used the logic in execute as the basis for the logic here. I noticed that in execute we make a copy of the UmpleOptions (in setOutputDirs we do UmpleOptions opts = newOpts). Is this copy necessary? I don't think we're changing the UmpleOption object.

99% sure that's referential copy so it does literally nothing.

AdamBJ · 2017-08-26T17:11:57Z

@Nava2 I've been looking into the Travis failure. The Linux build is working fine, but on Mac there's an assertion error here:

assertEquals("invalid outputDir", Paths.get(testProjectDir.root.toString(), "src/generated/\${language}"), Paths.get((String)props.get("umple.outputDir")))

I read on the travis site that: "Groovy builds are not available on the OS X environment." (https://docs.travis-ci.com/user/languages/groovy/). So how about we just remove the Mac stuff from Travis?

Nava2 · 2017-08-26T20:53:49Z

@AdamBJ I can't get to this till Tuesday I think. Defence + some personal things. Bump this if you don't hear from me by then, though.

Nava2 · 2017-08-26T20:54:48Z

Regarding the OSX build, it's probably installable via homebrew, brew install groovy.

AdamBJ · 2017-08-27T16:41:19Z

Good luck on Monday!

AdamBJ · 2017-08-30T18:28:49Z

Bump! Hope the thesis defence went well.

Nava2

Looking for some more feedback on this. But I'd also love to get this branch removed and some work into the master branch instead.

Nava2 · 2017-08-30T23:56:02Z

Base project/build.gradle

    classpath files('libs/umple-latest.jar')
    //TODO update {CURRENT_VERSION} to the version of the plugin you're using
    classpath "umple.gradle.plugin:UmpleGradlePlugin-{CURRENT_VERSION}" 
+    classpath group: 'com.google.guava', name: 'guava', version: '21.0'


Should probably make this 22.

Nava2 · 2017-08-30T23:57:16Z

libs/placeholder.txt

@@ -0,0 +1 @@
+Put the umple jar in this folder. It should be named umple-latest.jar.


We could use git-lfs if necessary here, but ideally we need to package umple for ivy/maven and ship it that way.

Nava2 · 2017-08-30T23:58:00Z

src/integrationTest/resources/TestProject/build.gradle

@@ -8,9 +8,8 @@ buildscript {
  dependencies {


Do we still need the integrationTest projects as we use the project via the gradle test runner?

I think we could convert it from the current style to the gradle test runner style we use for everything else (e.g. have the build file included within CompilationAndGeneration.groovy). I'm not 100% sure how it would work, though, since it's one of the more complicated tests (there are subprojects with their own build files, etc). I'd say it would be a "nice to have" feature to have a more consistent testing approach, but that it's probably not worth the effort to switch it over.

And to be clear, I don't think we use the integrationTest project outside of CompilationAndGeneration.groovy.

Yeah, I don't think its necessary -- we should be able to use the groovy test runner + temp folders.

Nava2 · 2017-08-30T23:58:46Z

src/main/groovy/cruise/umple/UmpleGradlePlugin.groovy

-            project.tasks.getByName(sourceSet.compileJavaTaskName).dependsOn umpleGenerate
+            // The user has requested that generated Java files are compiled. Make sure
+            // the compileJava task runs after the compileUmple task
+            umpleGenerate.finalizedBy project.tasks.getByName(sourceSet.compileJavaTaskName)


This is over using the umple compile mechanics? If so, I much prefer this -- use Gradle to it's advantage.

Yeah, when it comes to compiling Java files we don't (and never have as far as I can tell) use the Umple mechanic. This change makes it so the user can set the compileGenerated flag and invoke the compileUmple task in order to generate and compile. Before the user had to use compileJava if they wanted to compile the generated source. It made the compileGenerated flag redundant.

I think this should be on by default -- which it is! I can't think of a reason to have it be off by default.

Nava2 · 2017-08-31T00:02:39Z

src/main/groovy/cruise/umple/tasks/UmpleGenerateTask.groovy

+    void setOutputDirs(UmpleOptions newOpts) {
+        this.outputDirs = new ArrayList<File>()
+
+        UmpleOptions opts = newOpts


99% sure that's referential copy so it does literally nothing.

Nava2 · 2017-08-31T00:04:10Z

src/main/groovy/cruise/umple/tasks/UmpleGenerateTask.groovy

 /**
 * Created by kevin on 15/03/2017.
 */
 class UmpleGenerateTask extends SourceTask {


I don't think that this is properly implemented. I think Gradle needs to know all files that are generated. We either need to keep track of the files created or figure out a way to have Umple tell us what files it will change/create. The intention of the incremental updates is that gradle can choose whether or not to generate new files and also what files to delete.

Nava2 · 2017-09-02T14:43:40Z

I had this email marked unread because I had to do something.. but I don't know what. Is there something that's needed here other than the comments I've made? I can investigate the mac build.

Nava2 · 2017-09-02T14:44:56Z

Comment I didn't make before, change Base project folder to something like examples/ or similar. Just looks strange with all of the other structure.

AdamBJ · 2017-09-02T19:10:31Z

I had this email marked unread because I had to do something.. but I don't know what. Is there something that's needed here other than the comments I've made? I can investigate the mac build.

I think just that business with the incremental build. I'll have another look at that now. Great to see Travis green again!

Nava2 · 2017-09-02T19:58:31Z

I think just that business with the incremental build. I'll have another look at that now. Great to see Travis green again!

Still not green. :( There's an issue with tests on OS X, not sure why. I'm going to look into that when I finish some other work.

AdamBJ added 2 commits August 24, 2017 12:50

Refactoring, bug fixes

8f1dd0a

Improve README, fix grammatical errors in comments, fixed bug in UmpleGradlePlugin.groovy that was causing an IllegalArugmentException to be thrown when relativize was called, bug fixes relating to the integrationTest task.

Further README improvements

fba915d

AdamBJ requested a review from Nava2 August 24, 2017 20:13

AdamBJ commented Aug 24, 2017

View reviewed changes

Adding incremental build support to the UmpleGenerateTask

da81837

AdamBJ commented Aug 26, 2017

View reviewed changes

Nava2 reviewed Aug 31, 2017

View reviewed changes

Nava2 added 2 commits September 2, 2017 10:47

Rename Base project folder

d338bcf

Adjust build for OSX taken from assertj/assertj-generator-gradle-plugin

faf4fc1

Nava2 force-pushed the Summer_2017 branch from 968f00c to f334333 Compare September 2, 2017 14:56

Add OSX with updating the brew installations

1771817

Nava2 force-pushed the Summer_2017 branch from f334333 to 1771817 Compare September 2, 2017 14:58

Change back to matrix style declaration

e30e494



		// TODO this should not be neccesary. Find out why compileUmple is returning "UP-TO-DATE" when the output files have been deleted.
		compileUmple {

		@@ -0,0 +1 @@
		Put the umple jar in this folder. It should be named umple-latest.jar. No newline at end of file

Summer 2017 #22

Are you sure you want to change the base?

Summer 2017 #22

Uh oh!

Conversation

AdamBJ commented Aug 24, 2017

Uh oh!

AdamBJ Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdamBJ Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdamBJ commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdamBJ Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdamBJ Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdamBJ commented Aug 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nava2 commented Aug 26, 2017

Uh oh!

Nava2 commented Aug 26, 2017

Uh oh!

AdamBJ commented Aug 27, 2017

Uh oh!

AdamBJ commented Aug 30, 2017

Uh oh!

Nava2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nava2 Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

AdamBJ Aug 24, 2017 •

edited

Loading

AdamBJ Aug 25, 2017 •

edited

Loading

AdamBJ Aug 31, 2017 •

edited

Loading

AdamBJ Sep 1, 2017 •

edited

Loading

AdamBJ commented Aug 26, 2017 •

edited

Loading

Nava2 Aug 31, 2017 •

edited

Loading