Improve functionality for preparing tasks to be sent to sandbox#82
Improve functionality for preparing tasks to be sent to sandbox#82
Conversation
ljleppan
left a comment
ljleppan
left a comment
There was a problem hiding this comment.
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 :)
| try { | ||
| tarCreator.createTarFromProject(tempdir, tmcLangsPath, tmcRunPath, outputPath); | ||
| } catch (ArchiveException ex) { | ||
| java.util.logging.Logger.getLogger(TaskExecutorImpl.class.getName()).log(Level.SEVERE, null, ex); |
There was a problem hiding this comment.
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?
| */ | ||
| byte[] compressProject(Path path) throws IOException; | ||
|
|
||
|
|
tmc-langs-util/pom.xml
Outdated
| <groupId>junit</groupId> | ||
| <artifactId>junit</artifactId> | ||
| <version>RELEASE</version> | ||
| </dependency> |
There was a problem hiding this comment.
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.
| logger.error( | ||
| "An error occurred while preparing task.", e); | ||
| printErrAndExit( | ||
| "ERROR: Could not prepare task." |
There was a problem hiding this comment.
Most of the logger.error() and printErrAndExit() calls in this chunk of changes probably fit on single lines.
| return ProjectType.getProjectType(path).getLanguagePlugin(); | ||
| } | ||
|
|
||
|
|
| 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"); |
| import fi.helsinki.cs.tmc.langs.java.ant.AntPlugin; | ||
| import fi.helsinki.cs.tmc.langs.utils.TestUtils; | ||
|
|
||
| import java.io.*; |
| 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(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| import org.apache.commons.compress.archivers.ArchiveException; | ||
|
|
||
| import org.codehaus.plexus.util.FileUtils; | ||
|
|
There was a problem hiding this comment.
Are you sure? It's a unique domain, so it's surrounded by empty lines just like the other unique domains in the imports.
There was a problem hiding this comment.
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.
OAarne
force-pushed
the
prepareSandboxTask
branch
2 times, most recently
from
9628f92 to
607b323
Compare
|
Good job, LGTM! I'll let @nygrenh do the actual merge. |
nygrenh
force-pushed
the
prepareSandboxTask
branch
from
607b323 to
0abbdd2
Compare
3 participants
Add CLI command for preparing tasks to be sent to sandbox and implement the functionality in TaskExecutorImpl.