Improve functionality for preparing tasks to be sent to sandbox #82

OAarne · 2017-07-27T15:17:51Z

Add CLI command for preparing tasks to be sent to sandbox and implement the functionality in TaskExecutorImpl.

ljleppan

Thanks for the PR, looking good!

Most of my comments are super nitpicky stuff about whitespace and line widths.

The tar thing seems pretty tricky: I don't really have any good suggestions on how to deal with it. I'll be happy with anything that @nygrenh is happy with :)

ljleppan · 2017-07-27T19:51:23Z

tmc-langs-util/src/main/java/fi/helsinki/cs/tmc/langs/util/TaskExecutorImpl.java

+        try {
+            tarCreator.createTarFromProject(tempdir, tmcLangsPath, tmcRunPath, outputPath);
+        } catch (ArchiveException ex) {
+            java.util.logging.Logger.getLogger(TaskExecutorImpl.class.getName()).log(Level.SEVERE, null, ex);


Add a log message that says what project failed. Perhaps "Failed to create tar from project {}" where {} is either a project name, path or something like that?

ljleppan · 2017-07-27T19:52:06Z

tmc-langs-framework/src/main/java/fi/helsinki/cs/tmc/langs/LanguagePlugin.java

@@ -134,6 +134,8 @@ ValidationResult checkCodeStyle(Path path, Locale messageLocale)
     * @return The compressed file as a byte array.
     */
    byte[] compressProject(Path path) throws IOException;
+
+


Extra whitespace

ljleppan · 2017-07-27T19:55:22Z

tmc-langs-util/pom.xml

+            <groupId>junit</groupId>
+            <artifactId>junit</artifactId>
+            <version>RELEASE</version>
+        </dependency>


It's been a long time since I've actually compiled any code for this project: @nygrenh, are we running maven 2 or 3?

I'm asking 'cause maven 3 does not support the RELEASE metaversion. As there is already a dependency for junit on lines 46-50, I suggest we simply update the version number of that to the latest. That way we'll be compatible with both mvn2 and mvn3.

ljleppan · 2017-07-27T19:57:35Z

tmc-langs-cli/src/main/java/fi/helsinki/cs/tmc/langs/cli/Main.java

+            logger.error(
+                    "An error occurred while preparing task.", e);
+            printErrAndExit(
+                    "ERROR: Could not prepare task."


Most of the logger.error() and printErrAndExit() calls in this chunk of changes probably fit on single lines.

ljleppan · 2017-07-27T19:58:15Z

tmc-langs-util/src/main/java/fi/helsinki/cs/tmc/langs/util/TaskExecutorImpl.java

@@ -140,4 +153,6 @@ public void clean(Path path) throws NoLanguagePluginFoundException {
    private LanguagePlugin getLanguagePlugin(Path path) throws NoLanguagePluginFoundException {
        return ProjectType.getProjectType(path).getLanguagePlugin();
    }
+
+


Extra whitespace

ljleppan · 2017-07-27T19:59:12Z

tmc-langs-util/src/test/java/fi/helsinki/cs/tmc/langs/util/TaskExecutorImplTest.java

+        Path outputDirectory = Files.createTempDirectory("output-directory");
+        Path outputPath = new File("/tmp/output.tar").toPath();//outputDirectory.resolve("output.tar");
+        Path tmcRunPath = new  File("/home/local/onniaarn/Visulahti.R").toPath();//Files.createTempFile("fake-tmc-run", ".sh");
+        Path tmcLangsPath = new  File("/home/local/onniaarn/Projects/tmc-langs/tmc-langs-cli/target/tmc-langs-cli-0.7.7-SNAPSHOT.jar").toPath();//Files.createTempFile("fake-tmc-langs", ".jar");


These paths seem weird.

ljleppan · 2017-07-27T20:03:04Z

tmc-langs-util/src/test/java/fi/helsinki/cs/tmc/langs/util/TaskExecutorImplTest.java

+import fi.helsinki.cs.tmc.langs.java.ant.AntPlugin;
+import fi.helsinki.cs.tmc.langs.utils.TestUtils;
+
+import java.io.*;


Avoid wild-card imports.

ljleppan · 2017-07-27T20:04:25Z

tmc-langs-util/src/test/java/fi/helsinki/cs/tmc/langs/util/TaskExecutorImplTest.java

+        executor.prepareSandboxTask(exercisePath, submissionPath, outputPath, tmcRunPath, tmcLangsPath);
+        Path testDirectory = Files.createTempDirectory("test-directory");
+
+        Process untarringProcess = new ProcessBuilder("tar", "-C", testDirectory.toString(), "-xf", outputPath.toString()).start();


This is a tricky one. I don't think this'll work on Windows as-is, but at the same time any other solution is going to be incredibly annoying to implement and have other, almost as significant downsides.

Can we somehow check if tar is available on the system and only run the test then, perhaps outputting a warning or something such if it's not available?

ping @nygrenh

The offending test is now set to be skipped if the functionality is not available, i.e. when on Windows. Shouldn't be an issue since the code will only ever be deployed to Linux. This solution is @nygrenh approved.

ljleppan · 2017-07-27T20:06:08Z

tmc-langs-util/src/main/java/fi/helsinki/cs/tmc/langs/util/TaskExecutorImpl.java

@@ -16,13 +16,19 @@
 import com.google.common.base.Optional;

 import org.apache.commons.compress.archivers.ArchiveException;
+
+import org.codehaus.plexus.util.FileUtils;
+


Extra whitespace

Are you sure? It's a unique domain, so it's surrounded by empty lines just like the other unique domains in the imports.

Good catch. Seems that we've been using styles for imports even within this project. Better keep it the way it's now to keep the style consistent within the file.

ljleppan · 2017-07-28T16:57:54Z

Good job, LGTM!

I'll let @nygrenh do the actual merge.

ljleppan requested changes Jul 27, 2017

View reviewed changes

OAarne force-pushed the prepareSandboxTask branch 2 times, most recently from 9628f92 to 607b323 Compare July 28, 2017 13:08

ljleppan approved these changes Jul 28, 2017

View reviewed changes

nygrenh force-pushed the master branch from 89259d6 to 3e34e49 Compare October 6, 2017 19:22

Improve functionality for preparing tasks to be sent to sandbox

0abbdd2

nygrenh force-pushed the prepareSandboxTask branch from 607b323 to 0abbdd2 Compare December 18, 2017 09:04

Update checkstyle max line length

62def5b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve functionality for preparing tasks to be sent to sandbox #82

Improve functionality for preparing tasks to be sent to sandbox #82

OAarne commented Jul 27, 2017

ljleppan left a comment

ljleppan Jul 27, 2017

ljleppan Jul 27, 2017

ljleppan Jul 27, 2017

ljleppan Jul 27, 2017

ljleppan Jul 27, 2017

ljleppan Jul 27, 2017

ljleppan Jul 27, 2017

ljleppan Jul 27, 2017

OAarne Jul 28, 2017

ljleppan Jul 28, 2017

ljleppan Jul 27, 2017

OAarne Jul 28, 2017

ljleppan Jul 28, 2017 •

edited

Loading

ljleppan commented Jul 28, 2017 •

edited

Loading

Improve functionality for preparing tasks to be sent to sandbox #82

Are you sure you want to change the base?

Improve functionality for preparing tasks to be sent to sandbox #82

Conversation

OAarne commented Jul 27, 2017

ljleppan left a 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

ljleppan Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

ljleppan commented Jul 28, 2017 • edited Loading

ljleppan Jul 28, 2017 •

edited

Loading

ljleppan commented Jul 28, 2017 •

edited

Loading