Extract `NativeSourcePlugin` #2572

armanbilge · 2022-02-22T10:20:54Z

This PR branches off of #2566 to try extracting out a plugin for native sources as suggested in #2566 (comment).

This reverts commit 8a51945.

This reverts commit 045d714.

armanbilge · 2022-02-22T10:22:17Z

tools/src/main/scala/scala/scalanative/build/NativeSourcePlugin.scala

+
+}
+
+case object RustSourcePlugin extends NativeSourcePlugin {


I kept this here for now, but I guess the idea is this can move into another repo/library? :)

Yes, that would be the main goal. In my option, the perfect solution

Yes, I agree :) It can be complicated to modify the CI in the primary repo to accomodate rust's special requirements. I think all the rust integration can live very naturally in another repo: a compiler plugin, some std library interop like you suggested, etc.

armanbilge · 2022-02-22T10:23:50Z

tools/src/main/scala/scala/scalanative/build/NativeSourcePlugin.scala

+trait NativeSourcePlugin {
+
+  def extensions: Seq[String]
+
+  def compile(config: Config, path: Path): Option[CompilationContext]
+
+}


This is the new interface for compiling native sources.

armanbilge · 2022-02-22T10:24:30Z

tools/src/main/scala/scala/scalanative/build/NativeSourcePlugin.scala

+
+}
+
+sealed abstract class ClangSourcePlugin extends NativeSourcePlugin {


I hope this is not over-engineered 😅

armanbilge · 2022-02-22T10:25:18Z

tools/src/main/scala/scala/scalanative/build/NativeSourcePlugin.scala

+  /** Assembler file extension: ".S" */
+  val assemblerExt = ".S"
+
+  /** List of source patterns used: ".c, .cpp, .S" */
+  val extensions = Seq(cExt, assemblerExt)


I wasn't sure where .S should go, so I snuck it in here.

Also need to fix that comment :)

armanbilge · 2022-02-22T10:26:21Z

tools/src/main/scala/scala/scalanative/build/NativeConfig.scala

@@ -56,6 +56,8 @@ sealed trait NativeConfig {
   */
  def embedResources: Boolean

+  def nativeSourcePlugins: Seq[NativeSourcePlugin]


Is the NativeConfig the right place to keep these?

I think that could be the only way we could use to pass extensions without the usage of reflection.
I can imagine the following scenario:
We can define an sbt plugin, which could automatically populate some fields of nativeConfig,

object ScalaNativeRustIntegrationPlugin extends AutoPlugin { override def requires: Plugins = plugins.ScalaNativePlugin object autoImport { val rustIntegrationConfig = taskKey[RustPLuginConfig](???) val rustIntegrationPlugin = taskKey[RustIntegarionPlugin](???) case class RustPluginConfig() class RustIntegarionPlugin(config: RustPluginConfig) extends NativeSourcePlugin } }

and then in users code

val foo = project.in(file("foo"))) .enablePlugins(ScalaNativeRustIntegrationPlugin) .settings( rustIntegrationConfig := ???, nativeConfig ~= _.withNativeSourceCompilerPlugins(rustIntegrationPlugin.value) )

Yes, actually we can make it even easier for the user :)

object ScalaNativeRustIntegrationPlugin extends AutoPlugin { override def requires: Plugins = plugins.ScalaNativePlugin object autoImport { val rustIntegrationConfig = settingKey[RustPluginConfig](???) case class RustPluginConfig() } override def projectSettings = Seq( nativeConfig := { val plugin = new RustSourceCompilerPlugin(rustIntegrationConfig.value) nativeConfig.value.withNativeSourceCompilerPlugin(plugin) } ) }

Then user code is just:

val foo = project.in(file("foo")) .enablePlugins(ScalaNativeRustIntegrationPlugin) .settings( rustIntegrationConfig := ???, )

armanbilge · 2022-02-22T10:27:31Z

tools/src/main/scala/scala/scalanative/build/Discover.scala

+  def rustc: Path = discover("rustc", "RUSTC")
+  def cargo: Path = discover("cargo", "CARGO_HOME")


If the rust integration is extracted, I guess these should be moved/removed as well.

armanbilge · 2022-02-22T10:28:58Z

tools/src/main/scala/scala/scalanative/build/NativeConfig.scala

+      nativeSourcePlugins =
+        Seq(CSourcePlugin, CppSourcePlugin, RustSourcePlugin)


I put them all here for now, for backwards-compatibility. Maybe in future it should be empty, and default C/CPP plugins are populated elsewhere?

Maybe we can use a similar strategy as with linktimeProperties - the configurable part would include only custom plugins, while the core ones (C, Cpp,Rust?) would be always part of the core repo and always enabled.

You made another comment:

Maybe, if in the future someone would like to introduce better Cpp integration we could add settings to nativeConfig allowing for disabling our internal implementations.

I think instead of adding more settings, we should keep the C and Cpp plugins as part of the configurable list instead of force-enabling them. They can be included by default but then they will be easy to remove/replace. They should definitely remain part of the core repo always :)

I think the Rust makes sense as another repo.

armanbilge · 2022-02-22T10:30:19Z

tools/src/main/scala/scala/scalanative/build/LLVM.scala

+  ): Seq[CompilationResult] = {
+    import NativeSourcePlugin._
+
+    val plugins = LlSourcePlugin +: config.compilerConfig.nativeSourcePlugins


IIUC the .ll files are part of the core functionality—Scala Native will not work without them. So it seems that it should not be optional.

WojciechMazur

Great job on this topic, I really like the separation of compilation logic depending on the source.
I don't have today more time to spend on it, but it looks promising.

I wonder if in the final implementation we can merge some ideas from #2325 eg. filtering sources.

WojciechMazur · 2022-02-22T19:09:30Z

tools/src/main/scala/scala/scalanative/build/NativeConfig.scala

+
+  def withNativeSourcePlugins(plugins: Seq[NativeSourcePlugin]): NativeConfig
+
+  def withNativeSourcePlugin(plugin: NativeSourcePlugin): NativeConfig =
+    withNativeSourcePlugins(nativeSourcePlugins :+ plugin)


We can merge these two by using variadic arguments

Suggested change

def withNativeSourcePlugins(plugins: Seq[NativeSourcePlugin]): NativeConfig

def withNativeSourcePlugin(plugin: NativeSourcePlugin): NativeConfig =

withNativeSourcePlugins(nativeSourcePlugins :+ plugin)

def withNativeSourcePlugins(plugins: NativeSourcePlugin*): NativeConfig =

withNativeSourcePlugins(nativeSourcePlugins ++ plugins)

Actually, I wrote these with different purpose.

withNativeSourcePlugin appends a single plugin to your list of plugins.

withNativeSourcePlugins replaces the entire list of plugins. Otherwise there is no way to remove and/or change the order.

I guess it needs better names :)

I have some more thoughts on this topic, in the other comments.

WojciechMazur · 2022-02-22T19:16:02Z

tools/src/main/scala/scala/scalanative/build/NativeConfig.scala

+      nativeSourcePlugins =
+        Seq(CSourcePlugin, CppSourcePlugin, RustSourcePlugin)


Maybe we can use a similar strategy as with linktimeProperties - the configurable part would include only custom plugins, while the core ones (C, Cpp,Rust?) would be always part of the core repo and always enabled.

WojciechMazur · 2022-02-22T19:18:33Z

tools/src/main/scala/scala/scalanative/build/NativeConfig.scala

@@ -232,6 +248,7 @@ object NativeConfig {
        | - optimize:           $optimize
        | - linktimeProperties: $listLinktimeProperties
        | - embedResources:     $embedResources
+        | - nativeSourcePlugins:${nativeSourcePlugins.mkString(",")}


Maybe plugins should define their names (similarly to compiler phases), this way the output would be more friendly.
We can also wrap it into array and add space delimiter for better readability

Suggested change

| - nativeSourcePlugins:${nativeSourcePlugins.mkString(",")}

| - nativeSourcePlugins:${nativeSourcePlugins.mkString("[", ", ", "]")}

Yes, this should be improved 👍

WojciechMazur · 2022-02-22T19:37:33Z

tools/src/main/scala/scala/scalanative/build/NativeSourcePlugin.scala

+
+}
+
+case object RustSourcePlugin extends NativeSourcePlugin {


Yes, that would be the main goal. In my option, the perfect solution

WojciechMazur · 2022-02-22T19:41:19Z

tools/src/main/scala/scala/scalanative/build/NativeSourcePlugin.scala

+
+import NativeSourcePlugin._
+
+trait NativeSourcePlugin {


What do you think about a bit longer name?

Suggested change

trait NativeSourcePlugin {

trait NativeSourcesCompilerPlugin {

Yes, that seems good 👍

WojciechMazur · 2022-02-22T20:02:05Z

tools/src/main/scala/scala/scalanative/build/LLVM.scala

+  ): Seq[CompilationResult] = {
+    import NativeSourcePlugin._
+
+    val plugins = LlSourcePlugin +: config.compilerConfig.nativeSourcePlugins


According to the previous comment - if we would only define additional plugins in NativeConfig we should here define also CLang and Cpp plugins. Maybe, if in the future someone would like to introduce better Cpp integration we could add settings to nativeConfig allowing for disabling our internal implementations.

See #2572 (comment).

WojciechMazur · 2022-02-22T20:23:13Z

tools/src/main/scala/scala/scalanative/build/NativeConfig.scala

@@ -56,6 +56,8 @@ sealed trait NativeConfig {
   */
  def embedResources: Boolean

+  def nativeSourcePlugins: Seq[NativeSourcePlugin]


I think that could be the only way we could use to pass extensions without the usage of reflection.
I can imagine the following scenario:
We can define an sbt plugin, which could automatically populate some fields of nativeConfig,

object ScalaNativeRustIntegrationPlugin extends AutoPlugin { override def requires: Plugins = plugins.ScalaNativePlugin object autoImport { val rustIntegrationConfig = taskKey[RustPLuginConfig](???) val rustIntegrationPlugin = taskKey[RustIntegarionPlugin](???) case class RustPluginConfig() class RustIntegarionPlugin(config: RustPluginConfig) extends NativeSourcePlugin } }

and then in users code

val foo = project.in(file("foo"))) .enablePlugins(ScalaNativeRustIntegrationPlugin) .settings( rustIntegrationConfig := ???, nativeConfig ~= _.withNativeSourceCompilerPlugins(rustIntegrationPlugin.value) )

WojciechMazur · 2022-02-22T20:32:15Z

tools/src/main/scala/scala/scalanative/build/LLVM.scala

+      val inpath = path.toAbsolutePath()
+
+      val compiler = plugins
+        .find(p => p.extensions.exists(ext => inpath.toString.endsWith(ext)))


In the future, we might think about improvement for that, eg. building a Map (extension -> impl)

WojciechMazur · 2022-02-22T20:33:31Z

tools/src/main/scala/scala/scalanative/build/LLVM.scala

+      val inpath = path.toAbsolutePath()
+
+      val compiler = plugins
+        .find(p => p.extensions.exists(ext => inpath.toString.endsWith(ext)))


Also, we should check if we don't have clashes between plugins when two or more of them would use the same set of extensions

Probably a good idea :) I was thinking there can be some priority/precedence based on their ordering, but that has got limited use-case and potential for confusion probably :)

WojciechMazur · 2022-02-22T20:37:06Z

tools/src/main/scala/scala/scalanative/build/NativeLib.scala

-    val regexExtensions = srcExtensions.mkString("""(\""", """|\""", ")")
+  private def jarSrcRegex(plugins: Seq[NativeSourcePlugin]): String = {
+    val regexExtensions =
+      srcExtensions(plugins).mkString("""(\""", """|\""", ")")


There are a lot of plugins passing here, maybe we move all of this method to a single Impl taking and plugins list. This might allow for some optimizations also. In most cases, we would probably create at most 2 instances per run, (1 instance per findNativeLibs and findNativePaths call)

Oh yeah, that will be much better 😆

…ce-plugins

armanbilge · 2022-02-23T17:59:25Z

I wonder if in the final implementation we can merge some ideas from #2325 eg. filtering sources.

@ekrich do you know what the connection of this PR is to #2325? It wasn't obvious to me.

ekrich · 2022-02-23T20:08:58Z

I haven't had enough time yet to look at this PR. I don't think they are super related. Some of the parts of the build plugin concept had to do with filtering for windows that is now done in a different way and also conditional linking solved some of the things that were needed. I will put a comment on #2325 as an update.

ekrich · 2022-02-24T00:21:18Z

tools/src/main/scala/scala/scalanative/build/NativeConfig.scala

+        | - dump:                $dump
+        | - optimize:            $optimize
+        | - linktimeProperties:  $listLinktimeProperties
+        | - embedResources:      $embedResources


Removing the extra space here could reduce the diff,

I forgot I renamed the config in question. Actually we need more spaces 😆

ekrich · 2022-02-24T01:12:33Z

tools/src/main/scala/scala/scalanative/build/NativeSourcesCompilerPlugin.scala

+      case Mode.ReleaseFull => "-O3"
+    }
+
+  def compile(config: Config, inpath: Path): Option[CompilationContext] = {


Could you explain why this returns an option because in the C/C++ plugin it seems to be not needed?

That's a good question. I'm actually not 100% sure, this was the existing method signature. The rust compiler does return None in some instances. It seems to indicate that there is nothing to be done. But I'm not sure what differentiates it from returning a CompilationContext with an empty command. @WojciechMazur will probably know :)

scala-native/tools/src/main/scala/scala/scalanative/build/NativeSourcesCompilerPlugin.scala

Line 183 in 653dfab

if (!shouldBuildCrate) None

ekrich · 2022-03-14T23:05:31Z

tools/src/main/scala/scala/scalanative/build/LLVM.scala

+      compilationOutput.filter(_.isInstanceOf[ObjectFile]) ++
+        compilationOutput.filter(_.isInstanceOf[Library])


Can you use partition here to avoid 2 transits?

ekrich

Just a few small things - cosmetic. Will look more later.

ekrich · 2022-03-14T23:08:57Z

tools/src/main/scala/scala/scalanative/build/NativeLib.scala

    jarPattern.matcher(name).matches()
+  }


Spurious change?

ekrich · 2022-03-14T23:09:12Z

tools/src/main/scala/scala/scalanative/build/NativeLib.scala

-  private def readDir(path: Path): Option[Path] =
+  private def readDir(
+      path: Path
+  ): Option[Path] =


ekrich · 2022-03-14T23:09:26Z

tools/src/main/scala/scala/scalanative/build/NativeLib.scala

+      s"glob:${srcPathPattern(path)}**{",
+      ",",
+      "}"
+    )


ekrich · 2022-03-14T23:09:41Z

tools/src/main/scala/scala/scalanative/build/NativeLib.scala

+  private def destSrcPattern(
+      workdir: Path,
+      destPath: Path
+  ): String = {


…ce-plugins

WojciechMazur and others added 30 commits August 22, 2021 00:21

Rebase + add support for rust in native sources

41fa669

Add examples

a500fc5

Allow to compile cargo builds + minor refactor

763483f

Add unit test

722efc7

Clenaup

6c8cd64

Fix caching of already cached clang files

28f89bd

Scalafmt fix

80b17d6

Restore removed parallel collections compat

f0a8d7f

Fix partests

ec311b1

Remove not used sandbox files

a7eb599

Try install Rust on the CI

a4e1bb5

Pass -y flag to rust instlal script

a4094f0

Fix rust installation in docker iamge

fb5dd74

Fix typo in Dockerfile

ebedd42

Set explicit .cargo/bin diretory

39b3032

Merge branch 'master' into feature/rust-native-sources

88b8146

Enable ci

045d714

Merge branch 'main' into feature/rust-native-sources

9a39a7a

Fix bincompat

9fd65d0

Try a newer clang

bdd380d

Clang 12 it is

b808276

clang-11?

2bcbfa7

Try newer ubuntu

0a4c4c9

Try clang 12

2770a01

Restore current docker file

24123ef

Disable rust tests w/ LTO

8a51945

Fix compile

3729ff7

[ci skip] Revert "Disable rust tests w/ LTO"

ffbd11e

This reverts commit 8a51945.

[ci skip] formatting

513457f

Extract NativeSourcePlugin

f4fdbe5

armanbilge added 2 commits February 22, 2022 10:07

Cleanup and refactoring

2078a0f

Revert "Enable ci"

fc0d8fa

This reverts commit 045d714.

armanbilge commented Feb 22, 2022

View reviewed changes

armanbilge mentioned this pull request Feb 22, 2022

Refresh #2351 "Allow to embed Rust in native sources" #2566

Open

4 tasks

WojciechMazur reviewed Feb 22, 2022

View reviewed changes

armanbilge added 9 commits February 23, 2022 07:26

Merge remote-tracking branch 'upstream/main' into feature/native-sour…

7092814

…ce-plugins

NativeSourcePlugin -> NativeSourcesCompilerPlugin

198d8c0

Cleanups

79d3880

Use path to determine forceRebuild

4b912ae

Improve NativeConfig rendering

d066ad9

Use map for compilers, check for dupes

208d0b7

Create NativeLibUtil instances w/ plugin list

a8de72a

Rename config option

bf5d0ff

Add scaladocs

653dfab

armanbilge mentioned this pull request Feb 23, 2022

Refactor multi-arch CI setup #2571

Merged

ekrich reviewed Feb 24, 2022

View reviewed changes

[ci skip] Fix NativeConfig#toString alignment

245bb9b

ekrich reviewed Mar 14, 2022

View reviewed changes

armanbilge added 3 commits March 14, 2022 23:28

Optimization

6adbfea

Undo spurious changes

ecad3df

Merge remote-tracking branch 'upstream/main' into feature/native-sour…

8bb3aa3

…ce-plugins


		}

		sealed abstract class ClangSourcePlugin extends NativeSourcePlugin {

		def rustc: Path = discover("rustc", "RUSTC")
		def cargo: Path = discover("cargo", "CARGO_HOME")

		nativeSourcePlugins =
		Seq(CSourcePlugin, CppSourcePlugin, RustSourcePlugin)

	\| - nativeSourcePlugins:${nativeSourcePlugins.mkString(",")}
	\| - nativeSourcePlugins:${nativeSourcePlugins.mkString("[", ", ", "]")}

	trait NativeSourcePlugin {
	trait NativeSourcesCompilerPlugin {

		compilationOutput.filter(_.isInstanceOf[ObjectFile]) ++
		compilationOutput.filter(_.isInstanceOf[Library])

Extract NativeSourcePlugin #2572

Are you sure you want to change the base?

Extract NativeSourcePlugin #2572

Conversation

armanbilge commented Feb 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge Feb 23, 2022 • edited

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

WojciechMazur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge Feb 23, 2022 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge commented Feb 23, 2022

ekrich commented Feb 23, 2022

Choose a reason for hiding this comment

armanbilge Feb 24, 2022 • edited

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

ekrich 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

Extract `NativeSourcePlugin` #2572

Extract `NativeSourcePlugin` #2572

armanbilge Feb 23, 2022 •

edited

armanbilge Feb 23, 2022 •

edited

armanbilge Feb 24, 2022 •

edited