Add support for partially-shared source directories #145

armanbilge · 2022-02-19T10:09:51Z

Fixes #17.

For a full-cross-project set up for platforms a, b, c, d, etc. (in this order), configures the standard source and resource directories under these directories:

a-b
a-c
a-d
b-c
b-d
c-d
a-b-c
a-b-d
a-c-d
b-c-d

This is in addition to the already supported shared, a, b, c, and d directories.

armanbilge · 2022-02-19T10:11:11Z

sbt-crossproject/src/main/scala/sbtcrossproject/CrossProject.scala

+    }
+
+    private def makePartiallySharedSettings(platforms: Seq[Platform])(mkSettings: Seq[Platform] => Seq[Setting[_]]): Map[Platform, Seq[Setting[_]]] = {
+      platforms.to[ListSet] // preserve the platform ordering


I suppose another option here is to use SortedSet and impose a lexical ordering instead. This might be less surprising for users.

I would definitely recommend using lexicographic ordering, indeed. CrossProject is not really designed with the idea that the platforms are ordered in any way, so relying on that order is brittle.

Done, thanks.

armanbilge · 2022-03-01T23:52:13Z

@sjrd polite bump? :)

armanbilge · 2022-03-14T14:09:54Z

@sjrd I know sbt-crossproject PRs are no fun but whenever you can take a look would be much appreciated. Thanks! :)

sjrd · 2022-03-14T17:38:20Z

Right. I keep forgetting it. I'm on vacation this week but I'll have a look next week.

armanbilge · 2022-03-14T17:40:48Z

Have a nice vacation! :)

sjrd · 2022-03-21T13:09:05Z

...oject-test/src/sbt-test/new-api/partially-shared-sources/foo/shared/src/main/scala/Foo.scala

+    require(List(isJVM, isJS, isNative).count(identity) == 1)
+
+    if (isJVM)
+      require(JSOrJVM.check && JVMOrNative.check && !JSOrNative.check)


assert would make more sense than require here. Same for the two below.

sjrd · 2022-03-21T13:10:05Z

sbt-crossproject/src/main/scala/sbtcrossproject/CrossProject.scala

+import scala.collection.immutable.HashMap
+import scala.collection.immutable.ListSet
 import scala.language.implicitConversions


Suggested change

import scala.collection.immutable.HashMap

import scala.collection.immutable.ListSet

import scala.language.implicitConversions

import scala.language.implicitConversions

import scala.collection.immutable.HashMap

import scala.collection.immutable.ListSet

Language imports should always come first, in a dedicated "paragraph".

sjrd · 2022-03-21T13:17:12Z

sbt-crossproject/src/main/scala/sbtcrossproject/CrossType.scala

+      if (platforms.isEmpty) {
+        None


Every implementation will always have to do that. Instead, we should make it a pre-condition of partiallyShared{Src,Resources}Dir that platforms.nonEmpty. This is a bit more work for the (unique) caller of these methods, and less work for everyone who would like to implement CrossType.

This comment is still relevant, I believe.

Woops, I thought I got this. Must have been another one 🤔

sjrd · 2022-03-21T13:30:28Z

sbt-crossproject/src/main/scala/sbtcrossproject/CrossProject.scala

+        .map { platformSubset => // make the settings for this subset of platforms
+          val settings = mkSettings(platformSubset)
+          platformSubset.map(_ -> settings).toMap
+        }
+        .fold(platforms.map(_ -> Seq.empty).toMap)(mergeMapSeq)


This seems complicated and non intuitive to me. I think it may be reinventing a groupBy + flatMap. Can't it be rewritten as follows:

Suggested change

.map { platformSubset => // make the settings for this subset of platforms

val settings = mkSettings(platformSubset)

platformSubset.map(_ -> settings).toMap

}

.fold(platforms.map(_ -> Seq.empty).toMap)(mergeMapSeq)

.flatMap { platformSubset => // make the settings for this subset of platforms

val settings = mkSettings(platformSubset)

platformSubset.map(_ -> settings)

} // Seq[(Platform, Seq[Setting])]

.groupBy(_._1).map(kv => kv._1 -> kv._2.flatMap(_._2))

The last line would be easier with Scala 2.13's groupMapReduce, but we can't use it since this is 2.12 code. It would be written as

.groupMapReduce(_._1)(_._2)(_ ++ _)

The last line can even probably be delayed until the very end of shared{Src,Resources}Dir. Until then, we would keep using Seq[(Platform, Seq[Setting])] everywhere. This would entirely avoid passing through merge/toHashMap.

Thanks, this was a big big improvement 😅

sjrd · 2022-03-21T13:38:53Z

sbt-crossproject/src/main/scala/sbtcrossproject/CrossProject.scala

            )
          )
        }.toMap

      new CrossProject(id, crossType, projects)
    }

-    private def sharedSrcSettings(crossType: CrossType): Seq[Setting[_]] = {
+    private def sharedSrcSettings(
+        platforms: Seq[Platform],


This parameter is redundant. It is already accessible as an instance field.

sjrd · 2022-03-21T17:01:33Z

sbt-crossproject/src/main/scala/sbtcrossproject/CrossProject.scala

+        .map(_.toSeq.sortBy(_.identifier)) // use a consistent ordering
+        .flatMap { platformSubset => // make the settings for this subset of platforms
+          val settings = mkSettings(platformSubset)
+          platformSubset.map(_ -> settings).toMap


Suggested change

platformSubset.map(_ -> settings).toMap

platformSubset.map(_ -> settings)

sjrd · 2022-03-21T17:11:08Z

sbt-crossproject/src/main/scala/sbtcrossproject/CrossType.scala

+      if (platforms.isEmpty) {
+        None


This comment is still relevant, I believe.

armanbilge · 2022-03-21T17:35:17Z

@sjrd thanks for the review! Too much to ask for a 1.2.0? 😛

This commit adds the following changes - Move shared sources to js-native directory to reduce redundant files - See portable-scala/sbt-crossproject#145 - Port hash functions in pure Scala for js and native platform with reference to https://github.com/square/okio (Apache V2 License)

Bump version to 1.2.0-SNAPSHOT

fd991e8

armanbilge commented Feb 19, 2022

View reviewed changes

armanbilge force-pushed the feature/m-of-n-dirs branch 4 times, most recently from 7a96e24 to 4f50dc9 Compare February 21, 2022 13:36

sjrd requested changes Mar 21, 2022

View reviewed changes

armanbilge force-pushed the feature/m-of-n-dirs branch 2 times, most recently from 84c3b81 to b61c500 Compare March 21, 2022 14:33

armanbilge requested a review from sjrd March 21, 2022 14:50

sjrd requested changes Mar 21, 2022

View reviewed changes

Add support for partially-shared src dirs

06f7f75

armanbilge force-pushed the feature/m-of-n-dirs branch from b61c500 to 06f7f75 Compare March 21, 2022 17:16

sjrd approved these changes Mar 21, 2022

View reviewed changes

armanbilge mentioned this pull request Mar 21, 2022

[WIP] Add support for Scala native circe/circe#1929

Closed

4 tasks

sjrd merged commit 4ea98e3 into portable-scala:master Mar 21, 2022

xuwei-k mentioned this pull request Jun 16, 2024

use default sbt-crossproject source directories json4s/json4s#1479

Merged

i10416 mentioned this pull request Aug 22, 2024

feat: crypto hash functions(md5, sha1, sha256 and hmac) scodec/scodec-bits#513

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for partially-shared source directories #145

Add support for partially-shared source directories #145

armanbilge commented Feb 19, 2022

armanbilge Feb 19, 2022

sjrd Feb 21, 2022

armanbilge Feb 21, 2022

armanbilge commented Mar 1, 2022

armanbilge commented Mar 14, 2022

sjrd commented Mar 14, 2022

armanbilge commented Mar 14, 2022

sjrd Mar 21, 2022

sjrd Mar 21, 2022

sjrd Mar 21, 2022

sjrd Mar 21, 2022

armanbilge Mar 21, 2022

sjrd Mar 21, 2022

sjrd Mar 21, 2022

armanbilge Mar 21, 2022

sjrd Mar 21, 2022

sjrd Mar 21, 2022

sjrd Mar 21, 2022

armanbilge commented Mar 21, 2022

	platformSubset.map(_ -> settings).toMap
	platformSubset.map(_ -> settings)

Add support for partially-shared source directories #145

Add support for partially-shared source directories #145

Conversation

armanbilge commented Feb 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge commented Mar 1, 2022

armanbilge commented Mar 14, 2022

sjrd commented Mar 14, 2022

armanbilge commented Mar 14, 2022

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 Mar 21, 2022