Skip to content

Fix IR checking errors #74

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

Closed
wants to merge 2 commits into from
Closed

Conversation

sjrd
Copy link
Contributor

@sjrd sjrd commented Jul 14, 2017

Together with the Scala.js PRs scala-js/scala-js#3059 and scala-js/scala-js#3061, this fixes the IR checking errors visible in akkaJsActorJS/fastOptJS and akkaActorTestJS/test:fastOptJS.

Unfortunately we cannot enable IR checking in this build yet, because that will only work after upgrading to Scala.js 0.6.19, once it is released.

sjrd added 2 commits July 14, 2017 18:40
Instead of writing the class directory to a config file, and
*depending* on the `akkaJsActorIrPatches` project, we simply read
`products in (akkaJsActorIrPatches, Compile)`.

In addition to being substandard in terms of sbt task dependency
graph, the previous solution caused the classDirectory of the IR
patches to end up in `akkaJsActor`'s `fullClasspath` (despite the
`% "provided"`). Although this did not have an impact on published
artifacts (AFAICT), it polluted the set of IR files seen in this
build.
In the full sources, `Actor` is a `trait`, so its IR patch must
also be a `trait` to produce the correct IR nodes.
@andreaTP
Copy link
Member

thanks for your improvements,
I do not understand why there are tests failing in CI ...

@andreaTP
Copy link
Member

it turned out that turning Actor from class to trait breaks some tests

@andreaTP
Copy link
Member

Could I kindly ask to separate this into two PR so that I can immediately accept first one and, in the mean-while, find a fix for the second one please

@sjrd
Copy link
Contributor Author

sjrd commented Jul 17, 2017

Sure: #75.
The second commit on its own seems useless since it breaks things, so I don't think it's worth opening a PR for that one.

@andreaTP
Copy link
Member

sounds good, thanks

@andreaTP
Copy link
Member

things in this PR already got targeted.
I close it.

@andreaTP andreaTP closed this Sep 28, 2017
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.

2 participants