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

Added module for iron #692

Closed
wants to merge 12 commits into from
Closed

Added module for iron #692

wants to merge 12 commits into from

Conversation

rparree
Copy link

@rparree rparree commented Aug 3, 2023

Initial commit to add support for iron.

Originally i added a PR to the Iron project (Iltotore/iron#163), but it probably should be here.

I have been struggling with the build file and conflicts between cross compiling version. It might have to be double checked. Iron is a scala 3 project.

I have also not yet enabled support for JS.

@rparree
Copy link
Author

rparree commented Aug 4, 2023

I am having some difficulties troubleshooting the build. I already had to add two dummy subsequent git tags in order to make sure previousStableVersion was not None. When i run validate on the IronJVM project, it runs the test and works, but then breaks because mimaPreviousClassfiles, it is trying to obtain the previous published version of ciris-iron, which obviously is not yet available.

Copy link

@Iltotore Iltotore left a comment

Choose a reason for hiding this comment

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

Left some comments. Note I'm not familiar with Cirus neither SBT so I might be wrong.

build.sbt Outdated
name := moduleName.value,
dependencySettings ++ Seq(
libraryDependencies := Seq(
"io.github.iltotore" %% "iron" % ironVersion,
Copy link

Choose a reason for hiding this comment

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

In order to support JS and Native, you should use %%% instead of %% for Iron dependency.

build.sbt Outdated
@@ -182,6 +185,28 @@ lazy val refined = crossProject(JSPlatform, JVMPlatform, NativePlatform)
.nativeSettings(sharedNativeSettings)
.dependsOn(core)

lazy val iron = crossProject(JVMPlatform)
Copy link

Choose a reason for hiding this comment

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

To support JS and Native I guess you should add JSPlatform and NativePlatform.

You should also use jsSettings and nativeSettings.

Copy link
Author

Choose a reason for hiding this comment

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

tx!, I'll look into JS and native once i have JVM working

Copy link
Author

Choose a reason for hiding this comment

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

Decided to already add js/native as it does not seem to interfere with the problem i'm faced with

build.sbt Outdated
dependencySettings ++ Seq(
libraryDependencies := Seq(
"io.github.iltotore" %% "iron" % ironVersion,
"org.scalameta" %% "munit" % "1.0.0-M8" % "test",
Copy link

Choose a reason for hiding this comment

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

Shouldn't it be replaced with mimaSettings?

@rparree
Copy link
Author

rparree commented Aug 4, 2023

locally i am stuck on: Error downloading is.cir:ciris_2.13:3.2.0+62-5d77a72c+20230804-1312-SNAPSHOT

@Iltotore
Copy link

@rparree did you solve your problem?

If not, do the other modules compile successfully?

@@ -368,7 +401,7 @@ lazy val publishSettings =

lazy val mimaSettings = Seq(
mimaPreviousArtifacts := {
val unpublishedModules = Set[String]()
val unpublishedModules = Set[String]("ciris-iron")

Choose a reason for hiding this comment

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

Not sure what this code is supposed to do. As far as I can tell, this is the only difference with other modules. Maybe it is the cause of your dependency problem?

Copy link
Author

@rparree rparree Aug 17, 2023

Choose a reason for hiding this comment

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

This is not part of my commits, i believe this was part of a recent commit by vlovgr 55a3c98

Copy link
Owner

Choose a reason for hiding this comment

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

We add modules here when there's no published version yet, to exclude them from mima checks.

Copy link
Author

@rparree rparree Aug 17, 2023

Choose a reason for hiding this comment

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

OK that makes sense and explains why i was in a vicious circle.

@vlovgr
Copy link
Owner

vlovgr commented Aug 17, 2023

This shows up in the logs for the native version, not sure exactly what's causing this.

[error] Modules were resolved with conflicting cross-version suffixes in ProjectRef(uri("file:/home/runner/work/ciris/ciris/"), "ironNative"):
[error]    org.typelevel:cats-core_native0.4 _2.13, _3
[error]    io.github.cquiroz:cldr-api_native0.4 _2.13, _3
[error]    org.typelevel:cats-kernel_native0.4 _3, _2.13
[error]    org.typelevel:cats-effect-kernel_native0.4 _2.13, _3
[error]    io.github.cquiroz:scala-java-time_native0.4 _2.13, _3
[error]    io.github.cquiroz:scala-java-locales_native0.4 _2.13, _3

[error] java.lang.RuntimeException: Conflicting cross-version suffixes in: org.typelevel:cats-core_native0.4, io.github.cquiroz:cldr-api_native0.4, org.typelevel:cats-kernel_native0.4, org.typelevel:cats-effect-kernel_native0.4, io.github.cquiroz:scala-java-time_native0.4, io.github.cquiroz:scala-java-locales_native0.4

@rparree
Copy link
Author

rparree commented Aug 17, 2023

Locally, I see "conflicting cross-version suffixes" on cats-kernel errors on all three: JS, Native and JVM (when running sbt update). I am pretty sure i had tackled those before. I solved it then by using

    dependencySettings ++ Seq(
      libraryDependencies ++= Seq(
        ("io.github.iltotore" %%% "iron" % ironVersion).cross(CrossVersion.for2_13Use3),
      )
    ),
    scalaSettings ++ Seq(
      crossScalaVersions := Seq(scala3)
    ),

I revert back to that

@rparree
Copy link
Author

rparree commented Aug 17, 2023

Now i am left with:

[error] (ironJS / update) sbt.librarymanagement.ResolveException: Error downloading is.cir:ciris_sjs1_2.13:3.2.0+65-e0f19883-SNAPSHOT
[error]   Not found
[error]   Not found
[error]   not found: /home/rparree/.ivy2/local/is.cir/ciris_sjs1_2.13/3.2.0+65-e0f19883-SNAPSHOT/ivys/ivy.xml
[error]   not found: https://repo1.maven.org/maven2/is/cir/ciris_sjs1_2.13/3.2.0+65-e0f19883-SNAPSHOT/ciris_sjs1_2.13-3.2.0+65-e0f19883-SNAPSHOT.pom
[error] (ironJVM / update) sbt.librarymanagement.ResolveException: Error downloading is.cir:ciris_2.13:3.2.0+65-e0f19883-SNAPSHOT
[error]   Not found
[error]   Not found
[error]   not found: /home/rparree/.ivy2/local/is.cir/ciris_2.13/3.2.0+65-e0f19883-SNAPSHOT/ivys/ivy.xml
[error]   not found: https://repo1.maven.org/maven2/is/cir/ciris_2.13/3.2.0+65-e0f19883-SNAPSHOT/ciris_2.13-3.2.0+65-e0f19883-SNAPSHOT.pom
[error] Total time: 190 s (03:10), completed 17 Aug 2023, 11:08:30

@Iltotore
Copy link

You should not use Scala 2.x for Iron-related modules. It is Scala 3 only.

@rparree
Copy link
Author

rparree commented Aug 17, 2023

But we can use it in a 2.13 project using CrossVersion.for2_13Use3. This is how i got rid of the cross compilation dependencies on cats. AFAIK if we don't do this, we'll get two cats version on the dependency tree: the 2.13 from ciris and the 3.x one from iron.

@Iltotore
Copy link

But we can use it in a 2.13 project using CrossVersion.for2_13Use3

It shouldn't work because Iron relies heavily on Scala 3-only features like inline, S3 macros etc...

AFAIK if we don't do this, we'll get two cats version on the dependency tree: the 2.13 from ciris and the 3.x one from iron.

Then I don't get the .cross(CrossVersion.for2_13Use3) 🤔. If the module uses Scala 3, why would we need this?

@rparree
Copy link
Author

rparree commented Aug 18, 2023

@vlovgr why is the build trying to download ciris version ciris_2.13/3.2.0+65-e0f19883-SNAPSHOT. It would never find something like that as e0f19883 is my commit. Is this part of mima?

@vlovgr
Copy link
Owner

vlovgr commented Aug 19, 2023

@vlovgr why is the build trying to download ciris version ciris_2.13/3.2.0+65-e0f19883-SNAPSHOT. It would never find something like that as e0f19883 is my commit. Is this part of mima?

It is part of MiMa, but I'm not sure why it happens. If we check +mimaPreviousArtifacts there's no mention of that version, but +mimaReportBinaryIssues yields the resolution errors you mention. The version which fails to resolve is the current version (e0f1988) and it only seems to fail on Scala 2.12.x (2.13.x and 3.x work without issues). When I manually remove support for Scala 2.12.x in the build, it works as expected. It might be that we're hitting an issue in MiMa related to Scala 2.12.x support? If we can't find a workaround, I'm happy to drop Scala 2.12.x support in a separate pull request.

@Iltotore
Copy link

So, what is the state of this PR? Should the support for 2.12.x be dropped?

@vlovgr
Copy link
Owner

vlovgr commented Sep 10, 2023

So, what is the state of this PR? Should the support for 2.12.x be dropped?

I've opened #701 as a proposal and have asked for feedback. I haven't had a chance to look into workarounds, but also haven't really got any ideas. Maybe one alternative could be to have this module in the iron repository, similar to other iron modules?

@Iltotore
Copy link

Iltotore commented Sep 10, 2023

Actually, this was already suggested by @rparree but I suggested your repository instead, since the Refined module is also in yours.

I still prefer to have this module in this repository but if it happens to be too complicated to add, I can add it to Iron's repository.

I will try tomorrow to make this PR compile.

@Iltotore
Copy link

So I tried to fix this issue but I'm facing problems:

  • the previousArtifactVersion key is always None, causing NoSuchElementExceptions when loading the build script
  • my machine is not powerful enough to support the build. I always run out of memory and my CPU is overflowed.

Unfortunately, this last problem prevents me to further look for a fix.

Again, I hope someone will manage to make this PR compile but if it is too burdensome, I will merge the original PR in Iron's repository.

@Iltotore
Copy link

I finally decided to merge the original PR of @rparree in Iron's repository (Iltotore/iron#163)

@vlovgr
Copy link
Owner

vlovgr commented Oct 2, 2023

I finally decided to merge the original PR of @rparree in Iron's repository (Iltotore/iron#163)

That's great, thanks @Iltotore! 👍 Looks like we can close this pull request then.

@vlovgr vlovgr closed this Oct 2, 2023
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.

3 participants