Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override using directives scalac, java, and dependency options with their cli counterparts #612

Merged
merged 15 commits into from
Feb 8, 2022

Conversation

jchyb
Copy link
Contributor

@jchyb jchyb commented Feb 3, 2022

To achieve this for extraDependencies a Map was used, mapping the org + artifact names with their version, using already existing ConfigMonad structures for shadowing.

For cli-like options (for now scalac and java), a new collection-like structure was implemented, with an underlying Map mapping an option name to it's argument, called StringOptionsList. For now it supports three types of arguments - internally called "Spaced" (- XmaxWarns 5), "Coloned" (-g:none), and "Prefixed" (-Xmx2G). Prefixed arguments have to be built-in in scala-cli and are defined in a separate file. The good thing about this approach is that, if an argument of this type was missed, it simply will not be overridden, but still will be passed further to the compiler/jvm.
It is worth noting that this makes repeating options (like our -v -v) effectively unsupported in StringOptionsList, as those would all shadow each other, however I did not find any in scalac/java. If they are ever added, we can simply separately handle them, similarly to how "-Xmx" and other "Prefixed" are handled.

EDIT:
A ShadowingSeq collection was introduced, with ShadowingSeq[Positioned[AnyDependency]] for extraDependencies, ShadowingSeq[ScalacOpt] for scalacOptions and ShadowingSeq[JavaOpt] for javaOptions. Dependencies are overriden based on org name and artifact name. ScalacOpt and JavaOpt are overriden based on option name, for example -optionname arg1 can be shadowed by -optionname:arg2 arg3. ScalacOpt do not shadow options that can repeat (-Xplugin and -P:<>:<>), JavaOpt can have options shadowed based on a set of built-in, predefined prefixes (so for example -Xmx<value> will work)

@jchyb jchyb marked this pull request as draft February 3, 2022 14:35
Copy link
Contributor

@alexarchambault alexarchambault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fond of the new classes or type aliases TBH (DependencyMap and StringOptionsList). I'm pretty sure both could be implemented at once in a more elegant manner, with something along those lines:

/** Seq ensuring some of its values are unique according to some key */
final case class ShadowingSeq[T] private (values: Seq[T], key: ShadowingSeq.KeyOf[T]) {
  def ++(other: Seq[T]): ShadowingSeq[T] = {
    val l = new mutable.ListBuffer[T]
    val seen = new mutable.HashSet[String]
    for (t <- other.reverseIterator ++ values.reverseIterator) {
      val keyOpt = key.get(t)
      if (!keyOpt.exists(seen.contains)) {
        l += t
        for (key <- keyOpt)
          seen += key
      }
    }
    l.reverseIterator.toList
  }
}

object ShadowingSeq {
  final case class KeyOf[T](get: T => String)
  implicit def monoid[T](implicit key: KeyOf[T]): ConfigMonoid[ShadowingSeq[T]] =
    ConfigMonoid.instance(ShadowingSeq(Nil, key)) { (a, b) =>
      a ++ b.values
    }
}

Then we could introduce types for javac and scalac options, like

final case class JavacOpt(value: String) {
  def key: Option[String] = …
}
object JavacOpt {
  implicit val keyOf: ShadowingSeq.KeyOf[JavacOpt] =
    ShadowingSeq.KeyOf(_.key)
}
final case class ScalacOpt(value: String) {
  def key: Option[String] = …
}
object ScalacOpt {
  implicit val keyOf: ShadowingSeq.KeyOf[ScalacOpt] =
    ShadowingSeq.KeyOf(_.key)
}

and use those via ShadowingSeq[JavacOpt] and ShadowingSeq[ScalacOpt].

And for DependencyMap, we'd instead define an implicit KeyOf[Positioned[AnyDependency]] I guess, and use ShadowingSeq[Positioned[AnyDependency]].

(I didn't try to compile the code snippets above btw, so there might have typos or oversights…)

If that helps, we could also put something like this in the KeyOf or Positioned companion:

implicit def positioned[T](implicit underlying: KeyOf[T]): KeyOf[Positioned[T]] =
  KeyOf(pos => underlying.get(pos.value))

That way, we'd only need to define an implicit KeyOf[AnyDependency] rather than KeyOf[Positioned[AnyDependency]].


final case class ClassPathOptions(
extraRepositories: Seq[String] = Nil,
extraClassPath: Seq[os.Path] = Nil,
extraCompileOnlyJars: Seq[os.Path] = Nil,
extraSourceJars: Seq[os.Path] = Nil,
fetchSources: Option[Boolean] = None,
extraDependencies: Seq[Positioned[AnyDependency]] = Nil,
extraDependencies: DependencyMap = Map.empty,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we're losing the order of extraDependencies, which actually matters (it changes the order of JARs in the class path, which matters if two JARs define the same class or have resources at the same paths).

@@ -81,17 +82,23 @@ object ConfigMonoid {
main ++ defaults
}

implicit def map[K, V](implicit valueMonoid: ConfigMonoid[V]): ConfigMonoid[Map[K, V]] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely fishy. Some config values need maps to be "summed" the way they were before.

}.toMap
}

implicit def stringOptionsList: ConfigMonoid[StringOptionsList] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should live in the companion of StringOptionsList (we only put things that we can't change the companion of here).

@@ -47,6 +48,10 @@ object HashedType {
pf => pf.repr
}

implicit val stringOptionsList: HashedType[StringOptionsList] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, this should be in the companion of StringOptionsList.

Copy link
Contributor

@alexarchambault alexarchambault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(squash-and-merge if the CI is green)

@alexarchambault alexarchambault marked this pull request as ready for review February 7, 2022 18:57
@jchyb jchyb changed the title WIP: Override using directives scalac, java, and dependency options with their cli counterparts Override using directives scalac, java, and dependency options with their cli counterparts Feb 7, 2022
@alexarchambault alexarchambault merged commit 1347657 into VirtusLab:main Feb 8, 2022
romanowski pushed a commit to romanowski/scala-cli that referenced this pull request Mar 10, 2022
…heir cli counterparts (VirtusLab#612)

* Add initial String options and dependency shadowing mechanism

* Move implicits to seperate object

* Make scalac options Positioned and add prefixed option handling

* Shadow using directives java options with cli options

* Add tests for shadowing

* Apply scalafix

* scalafix

* Redesign shadowing for a more generic approach

* Revert changes to map ConfigMonoid

* scalafix

* fix compiler plugin option handling

* Simplify JavaOpt / ScalacOpt

They now only wrap a single option

* Better hashing for ShadowingSeq

* NIT / clean-up / diff minimization

Co-authored-by: Alexandre Archambault <alexandre.archambault@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuration from cmd should override using directives for dependencies
2 participants