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

Scala cross compilation #218

Merged
merged 2 commits into from
Dec 6, 2023

Conversation

semenodm
Copy link
Contributor

@semenodm semenodm commented Dec 3, 2023

No description provided.

@CLAassistant
Copy link

CLAassistant commented Dec 3, 2023

CLA assistant check
All committers have signed the CLA.

build.sc Outdated
extends JavaModuleTests
with ScalaVersionModule
with BaseMunitTests
}

object `readme-validator` extends BaseScalaNoPublishModule {
def moduleDeps = Seq(openapi, proto.core, `json-schema`)
object `readme-validator`
Copy link
Contributor

@Baccata Baccata Dec 4, 2023

Choose a reason for hiding this comment

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

no need to cross-build this one, it's only used to validate the code that's in the readme.

build.sc Outdated
@@ -377,7 +394,9 @@ object proto extends Module {
}
}

object examples extends BaseScalaModule with ScalaPBModule {
object examples extends Cross[ExamplesModule](scalaVersions)
Copy link
Contributor

Choose a reason for hiding this comment

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

probably no need to cross-build this one either.

import software.amazon.smithy.model.traits._
import software.amazon.smithy.model.traits.HttpTrait
import software.amazon.smithy.model.shapes._
import software.amazon.smithy.model.shapes.{Shape => JShape}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

.getOrElse(Set.empty)
.flatMap(_.keySet().asScala)
val auth = security.flatMap(securitySchemes.get).toVector
val authHint = if (auth.nonEmpty) List(Hint.Auth(auth)) else Nil
val schemes = securitySchemes.values.toVector
val securityHint =
if (schemes.nonEmpty) List(Hint.Security(schemes)) else Nil
val exts = GetExtensions.from(openApi)
val infoExts = GetExtensions.from(openApi.getInfo())
val exts = GetExtensions.from(openApi.asInstanceOf[HasExtensions])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should be able to avoid the runtime cast by changing the signature of from to be more 2.12-friendly, but I may be mistaken

Copy link
Contributor

@Baccata Baccata Dec 4, 2023

Choose a reason for hiding this comment

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

EDIT : yeah okay, I had forgotten it was a reflective call that we need because the openapi library is horrendous.

I guess we could centralise the cast in a def unsafeFrom(a: Any)

mutable.LinkedHashMap.from(all.map(a => a.id -> a))
all
.map(a => a.id -> a)
.foldLeft(new mutable.LinkedHashMap[DefId, Definition]()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

considering we're using a mutable structure, a good old foreach would have done : it's preferable to using a fold to mutate imho, as it's a better signal of mutability usage

Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively, use a ListMap I guess ? (hoping for it to be available in collections-compat)

allShapes
.foldLeft(new mutable.LinkedHashMap[DefId, Definition]()) {
case (acc, (id, shp)) =>
acc += (if (unused(id))
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto : if we'e gonna mutate, let's use a foreach

build.sc Outdated
)
}
}

object cli extends BaseScalaModule with buildinfo.BuildInfo {
object cli extends Cross[CliModule](scalaVersions)
Copy link
Contributor

Choose a reason for hiding this comment

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

arguably, we don't really need to cross-build this one as CLI usage, as CLIs shouldn't be consumed as libraries.

Copy link
Member

Choose a reason for hiding this comment

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

smithy-lang/smithy enters the room lol

Copy link
Contributor

Choose a reason for hiding this comment

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

loll, sure but it's a self contained fat jar
so cross built or not it would not matter

but I was thinking they would have dropped it by now

// ShapeBuilderOps[MemberShape.Builder, MemberShape](MemberShape
// .builder()
// .id(id.toSmithy)
// .target(tpe.toSmithy)).addHints(hints ++ jsonNameHint).build()
Copy link
Contributor

Choose a reason for hiding this comment

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

to remove, obviously

build.sc Show resolved Hide resolved
Not all modules should be cross compiled
@Baccata Baccata merged commit 1673e68 into disneystreaming:main Dec 6, 2023
3 checks passed
@semenodm semenodm deleted the scala-cross-compilation branch May 2, 2024 16:00
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

5 participants