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.

Sign up for GitHub

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

Jump to bottom

No longer use scalaVersionsByJvm #223

Merged
merged 5 commits into from
May 23, 2018
Merged

No longer use scalaVersionsByJvm #223

merged 5 commits into from
May 23, 2018

Conversation

lrytz
Copy link
Member

@lrytz lrytz commented May 18, 2018 •
edited
Loading

It was unnecessarily magic.

ashawley
ashawley reviewed May 19, 2018
View reviewed changes
.travis.yml
@@ -1,8 +1,20 @@
# opt-in to Travis's newer/faster container-based infrastructure
sudo: false

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should switch to Travis's newer Ubuntu precise infrastructure?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd stick with the defaults unless there's a good reason not to, but I also don't have a strong opinion. Either way is fine with me.

@ashawley ashawley mentioned this pull request May 19, 2018
Closed
@lrytz lrytz force-pushed the crossTravis branch 2 times, most recently from 67f919a to 74f3427 Compare May 19, 2018 05:46
@lrytz lrytz mentioned this pull request May 19, 2018
Closed
@lrytz lrytz force-pushed the crossTravis branch 8 times, most recently from 84f6ddb to f081fc6 Compare May 19, 2018 08:22
@lrytz
Copy link
Member Author

lrytz commented May 19, 2018

Rebased on master, this PR is now independent of #222.

@lrytz
Copy link
Member Author

lrytz commented May 22, 2018

Review by @SethTisue, @ashawley

@ashawley
Copy link
Member

So now close in favor of #222?

@lrytz
Copy link
Member Author

lrytz commented May 23, 2018

We can merge this first, independently of #222

ashawley
ashawley reviewed May 23, 2018
View reviewed changes
.travis.yml
@@ -11,18 +23,23 @@ env:
- secure: "OpBwPc1GNvauageYOH3RscAa7wpZxgpmqDz15aigIKLNWzAhAtVUx0MleZ8rQeoqml6nrAvlnzuVHjKL2lVcjMPpjUis7bcQ5UAGK7tZK8x+qZNQxXmpXu8+pENwQA2yFaqt/xy7K5jFOrHJHTRxcPnyVG1yKakPWz53PPYUwbc="
# SONA_PASS
- secure: "Xw7rI/qlML1nD2e2XwlakkhKAWNGZKqqE+Q3ntTvFpfHryl7KLCvVzJ4LIavnL6kGJaWOgy9vlSoEWn5g9nqHSfE31C/k5pY5nTMAKiwiJzfAS+r0asKXW2gmKhwtcTBkqyLVOZLCJSPVlFRQyfBJHY+Fs0L3KWcnMQgtBlyDhU="
matrix:
- SCALAJS_VERSION=
Copy link
Member

Choose a reason for hiding this comment

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

What's the significance of an empty SCALAJS_VERSION?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case it builds for the JVM. If SCALAJS_VERSION is non-empty, the build only builds for JS. See https://github.com/scala/scala-xml/pull/223/files#diff-3acefdae08499733e855dd23e1af933dR19.

On travis, every combination of the biuld matrix has its own "job", see https://travis-ci.org/scala/scala-xml/builds/381095011 for example.

Copy link
Member

Choose a reason for hiding this comment

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

Tricky.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is where the idea comes from: https://github.com/scala/scala-collection-compat/pull/20/files

Copy link
Member

Choose a reason for hiding this comment

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

Ok, we'll blame the code smell on @SethTisue, then. :-)

ashawley
ashawley reviewed May 23, 2018
View reviewed changes
.travis.yml Outdated
- oraclejdk8
- oraclejdk9
script:
- if [[ "$TRAVIS_JDK_VERSION" == "openjdk6" && "$TRAVIS_SCALA_VERSION" =~ 2\.11\..* || "$TRAVIS_JDK_VERSION" == "oraclejdk8" && "$TRAVIS_SCALA_VERSION" =~ 2\.1[23]\..* ]]; then export RELEASE_COMBO=true; fi
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this line is outside build.sh?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess my idea was to keep things that affect the build matrix in the travis file. But looking at it, I agree it's nicer to move it over to the bash script. Will do.

ashawley
ashawley reviewed May 23, 2018
View reviewed changes
.travis.yml Outdated

scala:
- 2.11.12
- 2.12.4
Copy link
Member

Choose a reason for hiding this comment

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

2.12.6???

ashawley
ashawley reviewed May 23, 2018
View reviewed changes
.travis.yml Outdated
exclude:
- scala: 2.13.0-M3
env: SCALAJS_VERSION=1.0.0-M3
- scala: 2.12.4
Copy link
Member

Choose a reason for hiding this comment

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

2.12.6, again?

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, sorry I missed these..

ashawley
ashawley reviewed May 23, 2018
View reviewed changes
project/plugins.sbt
Option(System.getenv("SCALAJS_VERSION")).filter(_.nonEmpty).getOrElse("0.6.23")

addSbtPlugin("org.scala-js" % "sbt-scalajs" % scalaJSVersion)
addSbtPlugin("org.portable-scala" % "sbt-scalajs-crossproject" % "0.4.0")
Copy link
Member

Choose a reason for hiding this comment

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

What's this about?

[warn] Found version conflict(s) in library dependencies; some are suspected to be binary incompatible:
[warn] 
[warn] 	* org.scala-js:sbt-scalajs:0.6.23 is selected over 0.6.22
[warn] 	    +- default:scala-xml-build:0.1-SNAPSHOT (scalaVersion=2.10, sbtVersion=0.13) (depends on 0.6.23)
[warn] 	    +- org.portable-scala:sbt-scalajs-crossproject:0.4.0 (scalaVersion=2.10, sbtVersion=0.13) (depends on 0.6.22)
[warn] 
[warn] 	* org.portable-scala:sbt-platform-deps:1.0.0 is selected over 1.0.0-M2
[warn] 	    +- org.scala-js:sbt-scalajs:0.6.23 (scalaVersion=2.10, sbtVersion=0.13) (depends on 1.0.0)
[warn] 	    +- org.portable-scala:sbt-crossproject:0.4.0 (scalaVersion=2.10, sbtVersion=0.13) (depends on 1.0.0-M2)
[warn] 	    +- org.portable-scala:sbt-scalajs-crossproject:0.4.0 (scalaVersion=2.10, sbtVersion=0.13) (depends on 1.0.0-M2)
[warn] 
[warn] Run 'evicted' to see detailed eviction warnings

Why doesn't the sbt-scalajs-crossproject plugin just bring in the corresponding sbt-scalajs core plugin?

Copy link
Member Author

Choose a reason for hiding this comment

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

sbt-scalajs-crossproject is not only about scala-js, but also native. I don't know it very well, but the docs say that the js plugin should be added (https://github.com/portable-scala/sbt-crossproject). It would be nice if this warning didn't show up though..

ashawley
ashawley reviewed May 23, 2018
View reviewed changes
.travis.yml
- scala: 2.12.6
jdk: openjdk6
- scala: 2.13.0-M3
jdk: openjdk6
Copy link
Member

@ashawley ashawley May 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

There are 19 builds at the moment. Do we really need all of them?

Trimming down would be nice, because the wall time used to be 10 minutes for 3 concurrent builds. The individuals builds are shorter, but now it looks like it takes over 20 minutes because Travis throttles only to 4 or 5 concurrent builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's not wrong to test them. Maybe we could skip combinations that contain two preview versions: Scala-js 1.0.0-M3 & openjdk9.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, sounds like a good start.

I'm thinking ahead to #110 when things will really go downhill.

@ashawley
Copy link
Member

Looks like the project list is changed:

> projects
[info] 	 * scala-xml
[info] 	   xml
[info] 	   xmlJS

Used to be:

> projects
[info] 	 * root
[info] 	   xmlJS
[info] 	   xmlJVM

We'll need to update the wiki when this is merged:

https://github.com/scala/scala-xml/wiki/Contributor-guide

@ashawley
Copy link
Member

Looks like a spurious Travis error for Scala 2.11.12, oraclejdk8 and scala-js 0.6.23.

No output has been received in the last 10m0s

@lrytz
Copy link
Member Author

lrytz commented May 23, 2018

I clicked "rebuild" on travis, it went green.

ashawley
ashawley reviewed May 23, 2018
View reviewed changes
admin/build.sh
fi

sbt "$publishVersion" "$publishScalaVersion" clean update +test +publishLocal $extraTarget
sbt "++$TRAVIS_SCALA_VERSION" "$publishVersion" "$projectPrefix/clean" "$projectPrefix/test" "$projectPrefix/publishLocal" "$publishTask"
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to see the update task go.

I don't know if "$projectPrefix/clean" is a worthwhile distinction.

But these are nits.

Copy link
Member Author

Choose a reason for hiding this comment

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

update runs anways as part of test

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I like the timestamp and success of each task I guess.

ashawley
ashawley approved these changes May 23, 2018
View reviewed changes
Copy link
Member

@ashawley ashawley left a comment

Choose a reason for hiding this comment

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

LGTM!

@lrytz
Copy link
Member Author

lrytz commented May 23, 2018

Yay! Thanks @ashawley for the review.

@lrytz lrytz merged commit 8a00d5f into scala:master May 23, 2018
This was referenced May 24, 2018
Closed
Closed
Merged
@ashawley ashawley added this to the 1.1.1 milestone Jun 25, 2018
@gourlaysama gourlaysama mentioned this pull request Oct 11, 2018
Merged
@SethTisue SethTisue mentioned this pull request May 30, 2019
Closed
@Philippus Philippus mentioned this pull request Jun 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashawley ashawley ashawley approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.1.1
Development

Successfully merging this pull request may close these issues.
2 participants
@lrytz @ashawley