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

Proposal to convert the main codebase to Scala #600

Closed
wants to merge 99 commits into from

Conversation

ekrich
Copy link
Contributor

@ekrich ekrich commented Nov 30, 2018

Proposal to convert the main codebase to Scala

Purpose

The initial idea was that lightbend/config is a dependency of scalafmt so if we change the API to Scala we could cross compile the library to Scala Native and then scalafmt could be compiled to Scala Native as well.

Process

The port was started with Config as that is the core interface in the library and given Config is an interface it could easily be converted to a trait and the process could move on. But true to form this interface has a Java varargs argument so this was a difficult start nonetheless. The port was done file by file until complete with the exception of the Java Optional annotation. Some process points are as follows:

  • The bulk of the conversion was done by IntelliJ’s converter from Java to Scala.
  • Modified the code as little as possible to compile and pass tests
  • Used import aliases for java.util as ju and java.lang as jl to highlight the code for better readability
  • Only moved inner classes/objects in one case where using from Java was not possible.
  • Commented all break and continue statements to make them easier to follow.
  • Made small changes to immutable here and there
  • Used scala.collection.JavaConverters for explicit conversions
  • Left/changed Scala methods without parameters without parentheses as this is more idiomatic for Scala
  • Did not change tests except for removing parens and using type ascriptions since Scala type safety is more robust

Deficiencies

  • The biggest issue is that creating Java enums in Scala is at best half baked. They work fine in Scala but are verbose and maintain the API faithfully but are not as useful in Java. Individually they can be used by adding the parentheses but cannot participate in switch statements without reverting to name() or ordinal() so if statements would be the best approach. They also are not marked as enums in byte code so they can not be used in and EnumMap but the implementation used a TreeMap which worked fine to preserve order.

  • The API has been maintained faithfully but the code will not be source compatible for either Java or Scala mostly do the parens in Scala and the enums in Java. The changes to adopt would be very easy to do however.

  • The Optional annotation and reflection based code would need to be evaluated in order to make something appropriate for Scala Native and Scala.JS. There is portable-scala-reflect but that would introduce a dependency and the library has none. Also, Scala Native does not support this reflection API yet.

  • Some access modifiers have been altered from the conversion. Some have been restored as they were commented out but some were left public in order to make the Java code using the Scala compile. The sheer bulk of code changed made perfection very difficult. Auditing the code could reintroduce tighter control where needed in the implementation code where the permissions were changed.

Conclusions and path forward

Certainly this was a significant amount of work so it should be in the community’s best interest to find a path forward if possible. One potential is to use this code as a path forward for Scala 2.11+ and above and only maintain bug fixes for the current version for current users of 2.10 through 2.12. This would allow a more flexible upgrade to newer versions for those customers on 2.11+ already.

One large benefit is that future developers can now code in Scala which is perhaps one of the biggest points for the maintainers.

Hopefully we can find a useful conclusion to this work that is completed to date.

* remove any deprecations and warnings
* upgrade lift-json to the last version for 2.10 and support the latest version for 2.11 and 2.12
* now will compile with listed versions in build file
@havocp
Copy link
Collaborator

havocp commented Dec 3, 2018

There were reasons for the Java implementation historically, I don’t know which still hold (I actually don’t remember the full rationale). In any case there are probably also a fair number of people using the library in plain Java apps these days.

Reimplementing in Scala I guess it is in theory possible to keep the ABI, but a new scala library dependency would be added even for Java apps.

I think if the goal were to replace the implementation of this library, the Play and Akka teams would be among those who need convincing. cc @2m @patriknw @viktorklang @jroper though I’m not sure of the current best contact. I would not unilaterally take this path without their buy in.

A less disruptive approach might be to try to share a test suite across distinct Java and Scala implementations that would be kept in sync by that suite. But I’m not sure how brutal the sbt wrangling would be to get to that. The current test suite of course requires providing a matching ABI and isn’t a TCK-style generic HOCON conformance suite.

I should caution that this library is under-maintained. The good thing is it works and has good tests. But, my work on it is basically limited to trying to respond to but not resolve or work on issues. I’m just trying to preserve institutional knowledge.

(My day job at the moment is all about getting libraries like this maintained, but I’m so busy trying to create a fully general solution to maintenance that I can’t do this particular maintenance 🙃)

@jroper
Copy link
Member

jroper commented Dec 3, 2018

Since this is a breaking change, and since it is so widely depended on by so many libraries, the only sane way to do this is to change the group id and package name. Otherwise, the entire ecosystem that uses Typesafe config, including parts of the ecosystem that are in maintenance mode or aren't even maintained any more but are still used, are going to have to do a big lockstep upgrade all at once, which is going to be hugely disruptive. For a library that itself is under-maintained, that's just not feasible, it's not like sbt where there was an existing large community of people that could embark on the massive task of upgrading the hundreds of downstream dependent projects, no such ecosystem for Typesafe config exists, it will be a mess.

But, if we change the package name, then both the old and new can coexist on the classpath, and all the downstream dependent libraries can upgrade at the leisure (or not, up to them). Larger projects like Play and Akka can even support both for a time to aid the transition. We can even continue to maintain the Java version on a branch for Java libraries that don't want to bring in Scala.

The most obvious choice for a package name, at least from my perspective, is com.lightbend.config, assuming this is going to stay as a Lightbend project.

And given that the change is going to be breaking, I'd say that Scala should be embraced in its fullest. For example, Scala doesn't really need enums, because we have pattern matching with case classes, so we should just use those. So we can also use scala.concurrent.Duration. Methods for the purpose of Java compatibility could be added (eg, do like Akka does in a lot of places, where x is for Scala, and getX is for Java, so stringList returns scala.collection.immutable.List while getStringList returns java.util.List).

@jroper
Copy link
Member

jroper commented Dec 3, 2018

All that said - my gut feel is if it ain't broke, don't fix. Lightbend doesn't have any need for this to be implemented in Scala, and in fact it's going to mean more unanticipated work for us because we'll have to upgrade and come up with a migration plan. So unless one of the tech leads speaks up to disagree, it's not likely that Lightbend is going to be contributing any effort towards this. @havocp are you considering doing the work to review this? It's not going to be a small task. Without either an existing maintainer such as @havocp or someone at Lightbend who is willing to review this work and ensure the approach make sense, that the Java API is workable, and everything is up to scratch, it's not likely to be merged.

@He-Pin
Copy link
Contributor

He-Pin commented Dec 3, 2018

@jvirtanen opps,they will depend on scala libraries which is currently not that small 😂

@He-Pin
Copy link
Contributor

He-Pin commented Dec 3, 2018

@jroper Seems like it could be a separate project with the sharing of tests.

@jroper
Copy link
Member

jroper commented Dec 3, 2018

Whether it's a separate project or not is not so much a technical question, as it is of what message we want to send and what expectations we want to set. Do we want to send the message that we expect that people will eventually "upgrade" to the Scala version? If so, it should be the same project, it can be made version 2, etc. Otherwise, if we expect most people to stay with the Java version, then it should be a separate project.

Personally, I'm in favour of viewing this as a new version of the same project, since it's already under maintained, as @havocp said, adding a new project will just spread the existing resources that maintain it even thinner. My concern is that if it's made an alternative project, then it'll just go there and die. Lightbend is not going to maintain it, we're not even giving the Java version the love it needs, and we're not likely to switch to a different version for no increased value. By making it a new version, it secures the future of it, because Lightbend will most likely upgrade all our projects (over time) to it, and so we'll do at least a minimum of maintenance on it over time.

If however it's viewed as an alternative port, with its own project, then it needs an owner, and that owner almost definitely won't be Lightbend.

@He-Pin
Copy link
Contributor

He-Pin commented Dec 3, 2018

Needs inputs from scalacenter too.
scalaifx @olafurpg

@havocp
Copy link
Collaborator

havocp commented Dec 3, 2018

@jroper no I can’t really review this (considering only JVM users, if I had time I’d love to catch up on dozens of other non-critical-but-real issues people have filed / PR’d here rather than push a giant reset button in effect and probably make a variety of new bugs).

It would probably also compromise my ability to do any maintenance at all if the code were all replaced - not that I’m doing a lot, but I try to help a little and reply to issues, since I know most of the tricky bits.

But I also understand if people are trying to clear out Java deps to allow Scala JS / Native projects (that’s the motivation right? has been my assumption).

I don’t want to block something if you all want to do it. Also I’d like to find a path where the native/JS folks have a viable way to help themselves.

I have no idea how popular non-JVM targets are right now or how artifact publishing works for them or anything else about those.

I appreciate the full-featured port concept, a certainly better path than alternatives with only half the spec implemented and a bunch of regressions.

But I wouldn’t want to swap out this implementation with that unless there were some credible plan to handle the resulting ripples of work.

Something alongside (used by native/JS only) would definitely be less disruptive I would think, and if adequate maintenance is eventually funded, it could be consolidated.

If the goal were simply a Scala API there are a bunch of wrappers already (see README), so those have been designed many times.
To me the goal here is probably drop-in replacement.

Anyway the library is in “it’s solid, it works, only a few conservative changes” mode - because that’s all the maintenance resources permit - if we wanted to make non-conservative changes that affect the JVM case I can’t commit to planning that or to managing any resulting fallout.

If there are ways to add a Scala version to the build and if it breaks only the non-JVM cases are affected, and the non-JVM community keeps it working; I wouldn’t be opposed fo having it in the source tree and sharing some tests, as a way to keep it in sync. But I’m not sure how viable that is.

@olafurpg
Copy link

olafurpg commented Dec 3, 2018

I am very excited about this port, thank you @ekrich for your hard work. Scalafmt and Scalafix use HOCON and I would love to run those applications on JS + Native. The lack of a high-fidelity HOCON Scala implementation has been a blocker for this to happen so far.

Migrating s/com.typesafe.config/org.scalahocon/ (or whatever package name) for cross-platform support is a small price to pay for Scalafmt/Scalafix. I prefer to use the same Scala implementation on all platforms (JVM+JS+Native) than the original Java implementation on the JVM and Scala port for other platforms.

For JVM-only users this change is arguably a regression so I share the hesitation with @havocp and @jroper to merge this port into the main repo, especially given the already limited resources to maintain the Java HOCON implementation.

My recommendation would be to setup a separate repo for this Scala port and release for JVM+JS+Native. This port can live under your personal fork @ekrich, a scalahocon/scalahocon organization or something else. I am happy to help out @ekrich as I'm eager to provide a native scalafmt binary :)

@dwijnand
Copy link
Member

dwijnand commented Dec 3, 2018

So, first things first, wow Eric! What an undertaking! Well done.

Certainly this was a significant amount of work so it should be in the community’s best interest to find a path forward if possible.

I really hope we do find a way forward, as it would be a massive shame to discard your efforts here. (In future I'd advise discussing this in advance unless you're happy to risk wasted effort.)

@mckoon:

What would be the implications for Java applications and libraries depending on this library?

As it stands this PR:

  • introduces the dependency on org.scala-lang:scala-library:2.10.7
  • changes the artifactId to config_2.10 while still containing classes under com.typesafe.config

The problem underlying this PR is the fact that:

  • JVM-based languages (Java, primarily) interoperate using compiled Java bytecode, while non JVM-based languages (Scala.js/Scala Native) interoperate using Scala source code, and
  • the Scala standard library isn't binary compatible across major versions (which in the Scala project is the nomenclature for the "Y" in version "X.Y.Z" - the "X" is "epoch")

So I would propose a different strategy than those discussed above.

What if we re-wrote this library in Scala but didn't depend on the scala library? I've never actually done this but I've been thinking about it for a long time. It would mean:

  • writing really weird Scala (e.g no scala.Option, no JavaConverters, etc)
  • careful, careful testing
  • conceivably quite a weird build setup to achieve it

But ideally it would achieve:

  • using the same artifactId without the Scala suffix
  • maintaining binary compatibility
  • maintaining source compatibility
  • adding support for Scala.js and Scala Native, which is the goal 😄

The one sticking point I can think of at this time is being able to write Java enums in pure Scala. I'm not sure how to do that, and we might need to use (or write) a custom scala compiler plugin to achieve that. Anyone know of anything available?

But true to form this interface has a Java varargs argument so this was a difficult start nonetheless.

I think that can be solved with Scala @varargs annotation:
https://www.scala-lang.org/api/2.12.x/scala/annotation/varargs.html

The Akka Team uses it in Akka or Akka-HTTP or something.

  • The API has been maintained faithfully but the code will not be source compatible for either Java or Scala mostly do the parens in Scala

From reading your commits I think you state you broke this to make the code more Scala-friendly. So we could also retain this source compatibility.

This current branch compiles in Scala 2.10. The only older feature used was a Manifest in one place as the newer solutions are 2.11+.

I'm not sure what you mean here. I looked quickly and it seemed you introduced Scala's Manifest usage in the library API, which we would want to revert in my hypothetical strategy.

@havocp

The current test suite of course requires providing a matching ABI and isn’t a TCK-style generic HOCON conformance suite.

As a secondary topic related here, it might be very useful if the test suite here were published in some conformance form such that other implementations could validate themselves against it.

@jroper (just an aside)

The most obvious choice for a package name, at least from my perspective, is com.lightbend.config, assuming this is going to stay as a Lightbend project.

👨‍🎨 s/config/HOCON/ please, mate 😁 (com.lightbend.hocon, org.scalahocon, or something)

@andreaTP
Copy link
Contributor

andreaTP commented Dec 3, 2018

This is really huge 🥇 @ekrich !
I really appreciate this initiative and I chip in since I already tried (and failed 😄 ) pushing something similar in the past, that eventually ended up in https://github.com/akka-js/shocon (that supports Scala.Js and Native).

To have a portable HOCON library I see one major blocking, as the current Typesafe Config implementation is heavily tangled with JVM, i.e. even if we manage to have a line-by-line port in Scala it won't easily link on the alternative platforms(that is the goal of this initiative). Concepts like ClassLoader and ResourcePath simply don't exists outside the JVM world and keeping API compatibility across platforms is pretty hard (if not impossible in certain cases).

The most straight forward step is, IMHO,to have, first, a separate test-suite to validate the adherence to the HOCON spec.

@havocp
Copy link
Collaborator

havocp commented Dec 3, 2018

Something to keep in mind, the Config and ConfigFactory and “hocon parser” bits of the library are three pretty distinct things.

Config is plain data - a JSON object plus withFallback and resolve operations. It can come from JSON, HOCON, Java properties (distinct parsers).

ConfigFactory is a convention about where to load configs from that enables multiple libraries in one app to share the same config file. Many of those conventions are JVM-specific.

The raw HOCON parser has a very limited public API (ConfigDocument) that people who truly want a HOCON parser tend fo find disappointing because it doesn’t expose format details. This lib mostly keeps fhe actual parser API private right now. One of the more popular feature requests is to expose more of it.

Kind of a side topic but. These are conceptually separable.

@ekrich
Copy link
Contributor Author

ekrich commented Dec 3, 2018

There is a bit too much response here for me to address all the comments but I'd like to address a few of the issues.

  1. The port is a faithful reproduction of the API but having @varargs on the Config interface introduces a extra Scala varargs method which is not on the original API. No other changes to the API were made.

  2. Reproducing Enums in Scala is not fully supported for Java usage but there are reproduced faithfully as the API includes a method def getEnum[T <: Enum[T]](enumClass: Class[T], path: String): T and also a list version. I think this is a weak point in Scala, Java interoperability.

  3. All the code is virtually the same but the Java code had break, continue so the breakable construct is used. Only in a few places is the code changed to be more Scala-like internally in the implementation but only because I couldn't figure out a good way to translate the code.

  4. Since Scala access modifiers are different, this has changed and rather than being private[impl] in the implementation package, some may be public - this is due to learning curve and sloppiness. API classes are the same.

Overall, I agree with the assessment that we will probably need to be a separate repo although I know it is always better to not have a fork. This is especially true if we have limited resources to help. The path forward for Java folks would be less that ideal in the current form. Support for Scala folks would be less than ideal as mentioned above.

@dwijnand This could be written without the Scala dependency but then we have no benefits of Scala and the API can't serve both Java and Scala both at a high level. I used Manifest in the implementation only. 2.10 does not support newer evidence types. I made sure that it does compile with Scala 2.11 and 2.12 in the latest commit.

@jroper I totally understand about resources, risk and plans. I do believe if we could make this a port that supports Scala on the JVM, Scala Native, and Scala.JS at a high level then perhaps it would be worth it for Lightbend, Akka, Play, etc. to adopt it in the future. Also, when this library was build and used originally, cross compiling and such was much more difficult than it is now thanks to the sbt team and community.

@havocp Thanks you for this library and as I said earlier that I don't think getting this far would have been possible without the excellent test coverage - tests in Scala - thank-you! I'm hoping That I can ask questions about features etc. as we move forward in some way or the other.

I would like to get the Scala Center's take where portable Scala code should reside. Certainly inputs from both @densh and @sjrd would be nice as well. I would also need a bunch of support from all the interested parties and those that have offered support such as @olafurpg, @gabro, @andreaTP, and @hepin1989 etc.

I think at a minimum, having a HOCON test suite that could support both should be goal. I think it is pretty clear that merging this code is less than ideal but I also want to let others weigh in before any final plan is made. In the meantime I can continue on a different branch working towards something that can be cross platform.

@sjrd
Copy link

sjrd commented Dec 3, 2018

Taking into account what has been said so far, my recommendation would be:

  1. Add the code of this PR as a separate sub-crossProject
  2. Include the sources of the existing test suite to the shared sources of that crossProject
  3. Publish everything (not tests)

This will allow several things:

  • Through the shared tests, we have a reasonable guarantee that all platforms work.
  • Tests will ensure that both versions stay in sync.
  • JVM-only projects can still depend on the Java version, so that they don't have the problem of transitively depending on scala-library.
  • Portable Scala projects can depend on the cross-project version.

Downsides are:

  • There is code duplication within this repo (but it's probably better than code duplication across repositories that are not guaranteed to be kept in sync)
  • Which means some duplicate work when there is maintenance in this repo.

@andreaTP
Copy link
Contributor

andreaTP commented Dec 3, 2018

I think at a minimum, having a HOCON test suite that could support both should be goal.

I totally agree 👍
Not sure if I will have time soon to actively work on anything related but don't hesitate to bother me if I can help (especially if any comparison/port from shocon is needed) my mail is public in github @ekrich

@havocp
Copy link
Collaborator

havocp commented Dec 3, 2018

@sjrd agree, I think adding a separate deliverable to this repo is a promising way to keep things in sync without disrupting JVM users. It would be good to add maintainers to this repo at the same time though!

@dwijnand
Copy link
Member

dwijnand commented Dec 3, 2018

@ekrich

@dwijnand This could be written without the Scala dependency but then we have no benefits of Scala and the API can't serve both Java and Scala both at a high level.

What do you mean no benefits? The benefit of it being in Scala is that it's cross-built into Scala.js and Scala Native.

And what do you mean by "the API can't serve both Java and Scala both at a high level"? I don't see how it can't serve as the same library.

Of course you can always wrap the library, but in the pursuit of adding Scala.js and Scala Native support I would keep out the desire to make it more "Scala-like".

@dwijnand
Copy link
Member

dwijnand commented Dec 3, 2018

@sjrd's idea is more easily implementable than mine and is better than the idea of a fork.

Maybe with Dotty we could eventually reconcile the two implementations back into one.

@nafg
Copy link

nafg commented Dec 4, 2018

Some random thoughts --

  1. Why is this a breaking change? Is it because some things in Scala just can't compile to the same API as before? Or just the fact that it adds scala-library to the classpath? I mean other than that couldn't it be a drop-in replacement?

  2. As @havocp said, the parser doesn't expose much API, yet it would seem that the parser's behavior is what's most "valuable" here. What if the parser were a separate module that was written in scala and cross-compiled, that might be easier to do without breaking any APIs.

  3. What about a Proguard or the like build step to get rid of scala-library and scala compatibility issues (at least for a Java-only audience build artifact)?

@ekrich
Copy link
Contributor Author

ekrich commented Dec 5, 2018

I really appreciate all the input from everyone.

The current config library is stable and serves it purpose well for Java and Scala customers alike and is deployed widely. Any changes or anything that could destabilize the project would be very detrimental to downstream projects. The project needs minimal support and introducing this code into the repository would be disruptive and since there is little need for this change and it would require resources, it is not desired.

The port, although a faithful reproduction of the current library, cannot serve the Java customer at the level of the current library for the following reasons.

  1. The enums are not recognized in Java and cannot participate in an EnumSet. The values are not constants so they cannot participate in an switch statement and an empty argument list is required to access the value.

  2. Currently the code uses the Scala Library which may not be required by a Java programmer. It may be possible to avoid this by not using any library features.

  3. There are other unresolved issues like the Java Annotation that may require small changes or omissions.

For the above reasons, we have decided to not pursue this PR. We will pursue a Scala, cross platform library with as much compatibility as possible. We will also cooperate on any issues that may affect both the Java and the Scala version. Again we appreciate all the comments and will cc the stakeholders.

cc/ @havocp @jroper @sjrd @olafurpg

@ekrich ekrich closed this Dec 5, 2018
@gabro
Copy link

gabro commented Dec 5, 2018

Thank you for sharing the decision @ekrich.

We will pursue a Scala, cross platform library with as much compatibility as possible

Can you update the issue here once there's something public to follow?

Also, it would be probably interesting to at least copy/adapt at least the test suite, in order to ensure API compatibility.

Thank you again for you work!

@ekrich
Copy link
Contributor Author

ekrich commented Dec 5, 2018

@gabro Sure no problem. We plan to use the test suite. Thanks for your kind comments as well.

@nafg
Copy link

nafg commented Dec 5, 2018 via email

@SethTisue
Copy link
Member

Eric has made https://github.com/ekrich/sconfig and published a release

@SethTisue
Copy link
Member

@ekrich may suggest you PR a change to the README in this repo, to call attention to your efforts?

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.

None yet