[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #486: Replace updateEclipseClasspaths with sbt eclipse plugin settings

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

mbeckerle commented on a change in pull request #486:
URL: https://github.com/apache/incubator-daffodil/pull/486#discussion_r568610805



##########
File path: build.sbt
##########
@@ -177,7 +182,18 @@ lazy val usesMacros = Seq(
   // ignores files such a META-INFA/LICENSE and NOTICE that are duplicated and
   // would otherwise cause a conflict
   mappings in (Compile, packageBin) ++= mappings.in(macroLib, Compile, packageBin).value.filter { case (f, _) => f.isDirectory || f.getPath.endsWith(".class") },
-  mappings in (Compile, packageSrc) ++= mappings.in(macroLib, Compile, packageSrc).value
+  mappings in (Compile, packageSrc) ++= mappings.in(macroLib, Compile, packageSrc).value,
+
+  // The sbt eclipse plugin does not addd the macroLib project as a dependency
+  // in .classpath files because macroLib isn't actually an sbt dependncy.
+  // Instead, sbt does the above stuff to copy macro lib files into those

Review comment:
       What makes macrolib "not an sbt dependency". I mean if you look above at the lazy val lib, it has
   ```
   .dependsOn(macroLib % "compile-internal, test-internal")
   ```
   which sure looks like an sbt dependency to me. So what do you mean by "isn't an sbt dependency"?
   
   So macrolib only contains macro definitions, so is treated this way to avoid a "real" dependency that would otherwise hang around and be required at runtime? If that's right, let's add that to the comments here.
   
   This page (below) describes sbt macro dependency. Is that what we're doing here? It's hard to tell reading that page, and reading build.sbt to recognize if we're using that same technique or not. 
   
   If what we're doing is the standard macro thing for sbt, we should say so in the comments, and if not we should clearly say so and why. 
   
   I'm surprised that sbt eclipse doesn't do the right thing if we're using the standard sbt technique for dealing with macros. 
   
   https://www.scala-sbt.org/1.x/docs/Macro-Projects.html
   
   If sbt eclipse isn't handling macros properly, then should we be reporting a bug in sbt eclipse, along with our "fix" for it? Or is what you have done sbt eclipse's way of handling this? I.e., it's not automatic, macros require use of hooks? 
   
   

##########
File path: build.sbt
##########
@@ -177,7 +182,18 @@ lazy val usesMacros = Seq(
   // ignores files such a META-INFA/LICENSE and NOTICE that are duplicated and
   // would otherwise cause a conflict
   mappings in (Compile, packageBin) ++= mappings.in(macroLib, Compile, packageBin).value.filter { case (f, _) => f.isDirectory || f.getPath.endsWith(".class") },
-  mappings in (Compile, packageSrc) ++= mappings.in(macroLib, Compile, packageSrc).value
+  mappings in (Compile, packageSrc) ++= mappings.in(macroLib, Compile, packageSrc).value,
+
+  // The sbt eclipse plugin does not addd the macroLib project as a dependency
+  // in .classpath files because macroLib isn't actually an sbt dependncy.
+  // Instead, sbt does the above stuff to copy macro lib files into those
+  // projects that need it. Because this copy doesn't happen for eclipse,
+  // eclipse can't find the macroLib files and leads to compilation errors. To
+  // fix this, we add an sbt eclipse transformer that adds the missing macroLib
+  // dependency to .classpath files only for these projects that use the macroLib
+  EclipseKeys.classpathTransformerFactories ++= Seq(
+    transformNode("classpath", DefaultTransforms.Append(EclipseClasspathEntry.Project(macroLib.base.toString))),

Review comment:
       btw: how does this avoid appending this classpath entry on the classpath for daffodil-macro-lib itself? 
   
   The EclipseClasspathEntry.Project(macrolib.base.toString) seems like the classpath entry to create. Not a filter. 
   
   Nothing else seems to filter out particular subprojects.
   
   Where is that specified?

##########
File path: build.sbt
##########
@@ -177,7 +182,18 @@ lazy val usesMacros = Seq(
   // ignores files such a META-INFA/LICENSE and NOTICE that are duplicated and
   // would otherwise cause a conflict
   mappings in (Compile, packageBin) ++= mappings.in(macroLib, Compile, packageBin).value.filter { case (f, _) => f.isDirectory || f.getPath.endsWith(".class") },
-  mappings in (Compile, packageSrc) ++= mappings.in(macroLib, Compile, packageSrc).value
+  mappings in (Compile, packageSrc) ++= mappings.in(macroLib, Compile, packageSrc).value,

Review comment:
       Typo "classe" in comment above. 

##########
File path: build.sbt
##########
@@ -15,6 +15,11 @@
  * limitations under the License.
  */
 
+// quiet errant sbt linter warnings about unused sbt settings

Review comment:
       Why is this needed? Why did "linter" and Global / excludeLintKeys get involved in this?
   The comment says what it does, but not why this is needed.




----------------------------------------------------------------
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