[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #465: Reorganize C source files

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org