Arbitrary arg length #574

himamis · 2016-01-04T12:54:09Z

This pull request fixes issue #572 .
A long term fix has been implemented, using a temporary file.
The test have also been modified to accommodate for the changes in the command line of the TranslateTask.

brunobowden · 2016-01-04T16:28:21Z

src/test/groovy/com/github/j2objccontrib/j2objcgradle/tasks/TranslateTaskTest.groovy

@@ -61,7 +61,7 @@ class TranslateTaskTest {
                '-d', '/PROJECT_DIR/build/j2objcSrcGenMain',
                '-sourcepath', '/PROJECT_DIR/src/main/java:/PROJECT_DIR/build/classes/main',
                '-classpath', '/J2OBJC_HOME/lib/j2objc_annotations.jar:/J2OBJC_HOME/lib/j2objc_guava.jar:/J2OBJC_HOME/lib/j2objc_junit.jar:/J2OBJC_HOME/lib/jre_emul.jar:/J2OBJC_HOME/lib/javax.inject-1.jar:/J2OBJC_HOME/lib/jsr305-3.0.0.jar:/J2OBJC_HOME/lib/mockito-core-1.9.5.jar:/J2OBJC_HOME/lib/hamcrest-core-1.3.jar:/J2OBJC_HOME/lib/protobuf_runtime.jar:/PROJECT_DIR/build/classes',
-                '/PROJECT_DIR/src/main/java/com/example/Main.java'
+                '@/PROJECT_DIR/build/tmp/j2objcTranslate/inputFiles'


Do you see how this loses something in the validation? Previously, it would check for Main.java but now that no-longer happens. Be very careful that you're not regressing the test. Please make two changes:

For short command lines, no temporary file should be used - so revert this test to what it was before. That preserves the existing behaviour and avoid the complexity and risk of the temporary file.

Add a specific test for the long command line. That'll need a normal test as usual with the @/PROJECT_DIR/build/tmp/j2objcTranslate/inputFiles, then an additional test that reads inputFiles and verifies the contents.

I'll make the changes. I also have a question:

What do you think, what should consist of a long command line? I thought having a an argument list with a length greater than 250000 is what should be called long (as noted before, for Mac OS X this is 262144). We could also tighten this number and make it around 100000.

brunobowden · 2016-01-04T20:49:24Z

@himamis - this should be enough feedback for you to fix this up. Thanks for working on this. Ping me when you've worked through this. There's a couple of other things for you to do:

Collapse the commits to a single commit - the contributor guide has notes on this
Make sure it passes on all the CI builds on each different platform
Add a note in the comment indicating how long the test takes to run (you can see it in the test output). If it's over a second then we should understand that and see if we can make it faster. I believe it should be pretty quick though.

himamis · 2016-01-05T08:54:51Z

@brunobowden please let me know if the changes are passable, then I'll prepare the pull request for submission.

brunobowden · 2016-01-06T06:14:56Z

src/test/groovy/com/github/j2objccontrib/j2objcgradle/tasks/TranslateTaskTest.groovy

                    final File file ->
+                    // Create the files
+                    file.write("// Fake")


Take this line out.

The file never needs to be written to disk in order for the test to work. In fact, writing a large number of files will slow down the test dramatically without adding anything to the quality of the testing. An important part of unit tests is that they should avoid anything like writing or reading to a file. In this case, just passing a list of "changed files" is enough for this test to verify the translate task.

Actually, I see the code that you moved here that was writing the file in another place. Can you check if it needs to write the file at all? I will test this myself also.

himamis · 2016-01-06T06:48:05Z

On Windows the test fails, if the files are not there:

junit.framework.AssertionFailedError: No call to 'delete' expected at this point. Still 1 call(s) to 'exec' expected.

Good news is that with Windows 240 files are enough, and with that the test is under 1 second on my machine.

brunobowden · 2016-01-06T06:59:15Z

src/test/groovy/com/github/j2objccontrib/j2objcgradle/tasks/TranslateTaskTest.groovy

@@ -189,18 +187,87 @@ class TranslateTaskTest {
        mockProjectExec.verify()
    }

+    @Test
+    // This test takes 8 seconds on average on a local dev computer


Remove this comment

brunobowden · 2016-01-06T07:09:57Z

@himamis - looks like the PR is close to complete. I see the same issue as you that it fails without any files. I put in a comment about trying it with just a main and test file. See if that works.

brunobowden · 2016-01-06T07:10:47Z

src/test/groovy/com/github/j2objccontrib/j2objcgradle/tasks/TranslateTaskTest.groovy

        proj.file('src/test/java/com/example').mkdirs()
-        proj.file('src/test/java/com/example/Verify.java').write("// Fake")


Add comment:

// File in this folder are created by 'genNonIncrementalInputs' method

himamis · 2016-01-06T07:12:23Z

Sorry, my comment was not clear enough. Without any files, all of the translate tests fail, with just Main1 and Verify1 only the translate_MaxArgsExceeded test fails.

brunobowden · 2016-01-06T07:15:08Z

Fair enough. In which case, leave it as setFakeOSWindows and creating all
the files. Then add a comment, in the genNonIncrementalInputs method that:

// TODO: prefer not to create all these temporary files just to run the

test

On Tue, Jan 5, 2016 at 11:12 PM Bencze Balázs [email protected]
wrote:

Sorry, my comment was not clear enough. Without any files, all of the
translate tests fail, with just Main1 and Verify1 only the
translate_MaxArgsExceeded test fails.

—
Reply to this email directly or view it on GitHub
#574 (comment)
.

himamis · 2016-01-06T08:09:12Z

@brunobowden The commits have been squashed into a single commit. Please let me know, if there is anything else I need to do.

brunobowden · 2016-01-06T08:14:09Z

src/main/groovy/com/github/j2objccontrib/j2objcgradle/tasks/TranslateTask.groovy

@@ -252,7 +252,7 @@ class TranslateTask extends DefaultTask {
                project.files(getTranslateSourcepaths()),
                project.files(getGeneratedSourceDirs())
        ])
-        doTranslate(sourcepathDirs, srcGenMainDir, translateArgs, mainSrcFilesChanged)
+        doTranslate(sourcepathDirs, srcGenMainDir, translateArgs, mainSrcFilesChanged, "mainSrcFilesArgsFile")


Replace "argsFile" with "argFile" to match the naming in the Java documentation. There's 13 instances of this.

Apologies I didn't catch this earlier:
http://docs.oracle.com/javase/7/docs/technotes/tools/windows/javac.html#commandlineargfile

brunobowden · 2016-01-06T08:16:45Z

Two minor nits and then we're done. Ping me once you've fixed them. I'll merge this in once all the CI builds pass. Thanks for your patience @himamis.

current OS Update TranslateTask, so that when the command line is too large, the file arguments are written into a temporary file, and they are fed to the j2objc command Add a new test case to test if the new files are created and they contain the correct arguments Add a new test case to test the Util method maxArgs()

himamis · 2016-01-06T08:45:53Z

@brunobowden The argsFile have been replaced with argFile. If there is anything else, let me know. Thank you in helping fixing this!

brunobowden · 2016-01-06T08:50:22Z

You're welcome, thank you too. I'll merge this in once the CI builds pass.

One last thing... Have you tested this to see if it makes your own build work now? The docs describe how you can test a locally built plugin for that build which was failing before:

https://github.com/j2objc-contrib/j2objc-gradle/blob/master/CONTRIBUTING.md#testing-on-your-own-gradle-project

Confirming that it works would make sure that this is going to solve your problem. I wanted to avoid the situation of releasing a new plugin version and then you find it doesn't solve your problem.

himamis · 2016-01-06T09:02:12Z

I've tested it, and I can confirm that with my project it works.

brunobowden · 2016-01-06T09:04:01Z

That's great @himamis. I'll merge this in the morning and then push a new public version for your delectation.

himamis · 2016-01-06T09:10:12Z

Thank you @brunobowden. It was fun working on this project! 😄

Arbitrary arg length

brunobowden reviewed Jan 4, 2016
View reviewed changes

brunobowden reviewed Jan 6, 2016
View reviewed changes

himamis force-pushed the arbitrary-arg-length branch from 5854d71 to 1a14d0d Compare January 6, 2016 08:05

brunobowden reviewed Jan 6, 2016
View reviewed changes

himamis force-pushed the arbitrary-arg-length branch from 1a14d0d to e00f068 Compare January 6, 2016 08:43

brunobowden added a commit that referenced this pull request Jan 6, 2016

Merge pull request #574 from himamis/arbitrary-arg-length

4646490

Arbitrary arg length

brunobowden merged commit 4646490 into j2objc-contrib:master Jan 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arbitrary arg length #574

Arbitrary arg length #574

himamis commented Jan 4, 2016

brunobowden Jan 4, 2016

himamis Jan 4, 2016

brunobowden commented Jan 4, 2016

himamis commented Jan 5, 2016

brunobowden Jan 6, 2016

brunobowden Jan 6, 2016

himamis commented Jan 6, 2016

brunobowden Jan 6, 2016

brunobowden commented Jan 6, 2016

brunobowden Jan 6, 2016

himamis commented Jan 6, 2016

brunobowden commented Jan 6, 2016

himamis commented Jan 6, 2016

brunobowden Jan 6, 2016

brunobowden commented Jan 6, 2016

himamis commented Jan 6, 2016

brunobowden commented Jan 6, 2016

himamis commented Jan 6, 2016

brunobowden commented Jan 6, 2016

himamis commented Jan 6, 2016

		proj.file('src/test/java/com/example').mkdirs()
		proj.file('src/test/java/com/example/Verify.java').write("// Fake")

Arbitrary arg length #574

Arbitrary arg length #574

Conversation

himamis commented Jan 4, 2016

brunobowden Jan 4, 2016

Choose a reason for hiding this comment

himamis Jan 4, 2016

Choose a reason for hiding this comment

brunobowden commented Jan 4, 2016

himamis commented Jan 5, 2016

brunobowden Jan 6, 2016

Choose a reason for hiding this comment

brunobowden Jan 6, 2016

Choose a reason for hiding this comment

himamis commented Jan 6, 2016

brunobowden Jan 6, 2016

Choose a reason for hiding this comment

brunobowden commented Jan 6, 2016

brunobowden Jan 6, 2016

Choose a reason for hiding this comment

himamis commented Jan 6, 2016

brunobowden commented Jan 6, 2016

himamis commented Jan 6, 2016

brunobowden Jan 6, 2016

Choose a reason for hiding this comment

brunobowden commented Jan 6, 2016

himamis commented Jan 6, 2016

brunobowden commented Jan 6, 2016

himamis commented Jan 6, 2016

brunobowden commented Jan 6, 2016

himamis commented Jan 6, 2016