stevedlawrence commented on a change in pull request #465:
URL: https://github.com/apache/incubator-daffodil/pull/465#discussion_r537494479
##########
File path:
daffodil-runtime2/src/test/scala/org/apache/daffodil/runtime2/TestCodeGenerator.scala
##########
@@ -26,18 +26,31 @@ import org.apache.daffodil.compiler.Compiler
import org.apache.daffodil.util.Misc
import org.apache.daffodil.util.SchemaUtils
import org.apache.daffodil.util.TestUtils
+import org.junit.After
import org.junit.Assert.assertArrayEquals
+import org.junit.Before
import org.junit.Test
/**
* Checks that we can create a [[CodeGenerator]] and call its methods.
* The value of this test is to debug the call path from [[Compiler]]
- * to [[CodeGenerator]] for one very simple DFDL schema. Running TDML
- * tests with daffodil-runtime2 is a more effective way to test the
- * functionality of CodeGenerator's generated code for as many DFDL
- * schemas as you could want.
+ * to [[CodeGenerator]] for a single test DFDL schema. Running TDML
+ * tests with daffodil-runtime2 is a more effective way to check that
+ * CodeGenerator can generate appropriate code for as many DFDL schemas
+ * as you could want.
*/
class TestCodeGenerator {
+ // Ensure all tests remove tmpOutputDir after using it
+ val tmpOutputDir: os.Path = os.pwd/"generate_tmp"
Review comment:
Any particular reason to not use ``Files.createTempDirectory``? That has
the advantage that it goes in /tmp, and ensures theat you have a unique, empty
directory. Avoids the need for the before function to clean it out. I also
noticed one of the windows tests failed in here somewhere with an error about
this "generate_tmp" directory. I wonder if there is some kind of race condition
here?
##########
File path:
daffodil-runtime2/src/test/scala/org/apache/daffodil/runtime2/TestCodeGenerator.scala
##########
@@ -78,35 +91,29 @@ class TestCodeGenerator {
val cg = pf.forLanguage("c")
// Generate code from the test schema successfully
- val outputDir = cg.generateCode(None, "./generateCode_tmp")
+ val outputDir = cg.generateCode(None, s"$tmpOutputDir")
Review comment:
Thoughts on having each test create it's own temp directory? That way
there's no possibility for one test to interfere with another test?
I've also wished for a while that we had a function that all our tests could
use so we could have a standard way to create temp directories. Right now lots
of tests do it differently and we pollute /tmp with a bunch of
files/directories. It would be nice if had something like this instead:
```scala
object DaffodilUtil {
private val rootTempDir = Paths.get(System.getProperty("java.io.tmpdir"),
"daffodil")
def withDaffodilTempDir[T](prefix: String = "")(body: Path => T): T = {
Files.createDirectories(rootTempDir)
val tmpDir = Files.createTempDirectory(rootTempDir, prefix)
try {
body(tmpDir)
} finally {
// recurisively delete tmpDir
}
}
}
```
Then, we can update individual tests, and maybe even things like the TDML
runner, that need a temp directory can just do something like:
```scala
DaffodilUtil.withTempDir() { tmpDir =>
// do test using tmpDir, it will automatically be deleted when complete
}
```
Might be out of scope for this change though. If you don't want to add
somethign like this to this PR, I'll create a bug so we don't forget. I've had
this in the back of my head for a while.
##########
File path: build.sbt
##########
@@ -57,13 +57,15 @@ lazy val runtime2 = Project("daffodil-runtime2",
file("daffodil-runtime2
Compile / ccTargets := ListSet(runtime2CFiles),
Compile / cSources := Map(
runtime2CFiles -> (
- ((Compile / resourceDirectory).value / "c"
* GlobFilter("*.c")).get() ++
+ ((Compile / resourceDirectory).value / "c"
/ "libcli" * GlobFilter("*.c")).get() ++
+ ((Compile / resourceDirectory).value / "c"
/ "libruntime" * GlobFilter("*.c")).get() ++
Review comment:
Just an FYI, you can use ** to get a recusrive glob, for example, you
could instead do something like:
```scala
((Compile / resourceDirectory).value / "c" / ** GlobFilter("*.c")).get()
```
Though, being explicit might have some advantages?
Another question, since the libcli and libruntime directories are both added
to runtime2CFiles, does that mean both libs end up in the same .so/.a files?
Same goes for the examples file? If we're separating out source, do we also
want to separate the resulting compiled objects? Or is this purley
organizational?
Another related question, I see we are also compiling the examples files
into this same .a/so file? Should that be separate as well? And chance of
conflicts between examples, libcli, and libruntime?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
