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

Add Scala.js cross build #35

Merged
merged 3 commits into from
Jul 29, 2019
Merged

Add Scala.js cross build #35

merged 3 commits into from
Jul 29, 2019

Conversation

exoego
Copy link

@exoego exoego commented Jul 24, 2019

Closes #30

.travis.yml Outdated Show resolved Hide resolved
@exoego exoego force-pushed the scalajs branch 2 times, most recently from 5e60c1e to a14f62b Compare July 24, 2019 04:31
Copy link
Member

@SethTisue SethTisue left a comment

Choose a reason for hiding this comment

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

the build.sbt changes LGTM

I don't think the .travis.yml matrix is right; I looks to me like it currently results in only JS getting built/tested, JVM is skipped. see the matrix: section here: https://github.com/scala/scala-parser-combinators/blob/1.2.x/.travis.yml

@SethTisue
Copy link
Member

(sorry, needs rebase because of #39)

@exoego
Copy link
Author

exoego commented Jul 25, 2019

@SethTisue Rebased and followed the convention in scala-parser-combinator.

build.sbt Outdated
.aggregate(`scala-collection-contribJS`, `scala-collection-contribJVM`)
.settings(disablePublishing)

lazy val `scala-collection-contrib` = crossProject(JVMPlatform, JSPlatform)
Copy link
Collaborator

Choose a reason for hiding this comment

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

May I suggest using a CrossType.Pure project type?

Copy link
Author

@exoego exoego Jul 25, 2019

Choose a reason for hiding this comment

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

I tried, but encountered test compilation error during root/test, though fine with individual scala-collection-contribJ(S|VM)/test.

Copy link
Author

@exoego exoego Jul 27, 2019

Choose a reason for hiding this comment

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

Fixed and now Pure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Weirdly, it seems that e15c87d has been overridden by another commit.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, it was my mistake.
test or root/test still causes error as mentioned above, so removed the commit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, I’ve just tried and observed the same behavior as you. Any idea, @SethTisue or @sjrd? (I guess the problem might come from sbt-scala-module or sbt-crossproject?). In the meantime I’ve found a workaround (see exoego#1)

Copy link
Member

Choose a reason for hiding this comment

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

Relying on aggregates with a Pure cross-project in(file(".")) leads to issues, because the default root project created by sbt will also be in(file(".")) and hence will try to pick up the sources and tests of the cross-project, leading to weird issues because the root project is not configured properly (for example it will be missing dependencies).

The solution is not to rely on aggregates. Rewire the default test command instead, or specify a project in which sbt should execute tasks by default that is different from the root project.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, right, I missed the fact that both projects were in(file(".")). We should just use a different root directory for the cross project, then.

Copy link
Member

Choose a reason for hiding this comment

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

(I merged regardless, but of course a followup PR would be welcome.)

julienrf and others added 2 commits July 29, 2019 11:22
I had to add a dependency on JUnit to the root project, otherwise
the aggregated projects failed compiling.
@SethTisue SethTisue merged commit 1988581 into scala:master Jul 29, 2019
@exoego exoego deleted the scalajs branch July 29, 2019 22:14
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.

add Scala.js cross-build
4 participants