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

support sbt 1.0.0-RC3 #49

Merged
merged 1 commit into from
Aug 7, 2017
Merged

Conversation

xuwei-k
Copy link
Contributor

@xuwei-k xuwei-k commented Jul 17, 2017

#48

Copy link
Collaborator

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Wow thanks! Here I thought I knew what I was going to do tomorrow, but now it's all done!

I have a few remarks below.

_.key.key.label == libraryDependencies.key.label) ++ Seq(
// https://github.com/sbt/sbt/issues/3325
libraryDependencies ++= {
CrossVersion.binarySbtVersion(scriptedSbt.value) match {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentation issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

formatted by scalafmt :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/scala-native/sbt-crossproject/blob/447264df1df049d05f83266eac07e1200b458c6d/bin/scalafmt#L13

$HERE/.coursier bootstrap com.geirsson:scalafmt-cli_2.11:0.4.7 --main org.scalafmt.cli.Cli -o $HERE/.scalafmt

Should we use latest version of scalafmt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it fixes the indentation problem here, I would say yes.

.travis.yml Outdated
@@ -69,6 +69,7 @@ before_script:
script:
- java -version
- bin/scalafmt --test && sbt sbt-crossproject-test/scripted
- sbt "^^ 1.0.0-RC2" sbt-crossproject/compile
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you make it sbt-crossproject/publishLocal so that we make sure that all the artifacts necessary for online publishing will work? In my experience generating documentation can often break while the rest passes, under -Xfatal-warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,6 @@
package sbt
Copy link
Collaborator

Choose a reason for hiding this comment

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

We really should not put anything in the sbt package. There's no excuse for this. Other plugins could be tempted to do the same, and then you have the same class defined twice on the classpath.

IIRC that method, StringUtilities.nonEmpty, was very short. I suggest to copy it and integrate it as private inside sbt-crossproject, and stop using the one coming from sbt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you. I will update 🙆‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

val sbtPluginSettings = ScriptedPlugin.scriptedSettings ++ Seq(
val sbtPluginSettings = ScriptedPlugin.scriptedSettings.filterNot(
_.key.key.label == libraryDependencies.key.label) ++ Seq(
// https://github.com/sbt/sbt/issues/3325
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure I understand what's going on, here? Is the workaround implemented here sufficient to get scripted to work with 1.0.0-RC2? If yes, why doesn't .travis.yml run scripted for the plugin under 1.0.0-RC2? If not, what is the purpose of these settings here?

Copy link
Contributor Author

@xuwei-k xuwei-k Jul 18, 2017

Choose a reason for hiding this comment

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

We can't execute scripted tests because sbt-crossproject-test and sbt-scalajs-crossproject depends on "org.scala-js" % "sbt-scalajs" % "0.6.16" which does not support sbt 1.0.0-RC2.

Is the workaround implemented here sufficient to get scripted to work with 1.0.0-RC2

You’re almost right. but without this workaround, sbt throw scala.MatchError when just execute sbt "^^ 1.0.0-RC2" not scripted.

BTW, sbt-crossproject does not have scripted tests yet. Should we remove scripted tests settings from
sbt-crossproject? This workaround unnecessary if remove scripted tests settings from
sbt-crossproject

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK that makes sense.

I will let @densh decide whether removing the scripted settings from sbt-crossproject makes sense. I am not too familiar with the consequences here.

@sjrd
Copy link
Collaborator

sjrd commented Jul 18, 2017

Apart from the scalafmt thing, it's good for me :) @densh should have a final look at it.

@@ -1 +1 @@
sbt.version=0.13.15
sbt.version=0.13.16-M1
Copy link
Collaborator

Choose a reason for hiding this comment

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

sbt 0.13.16 final has been published. Could you amend your PR to use that version, please?

@xuwei-k xuwei-k force-pushed the sbt-1.0.0-RC2 branch 2 times, most recently from 9d09c01 to 87c4950 Compare July 28, 2017 13:08
Copy link
Collaborator

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Thanks!

@xuwei-k xuwei-k changed the title support sbt 1.0.0-RC2 support sbt 1.0.0-RC3 Jul 30, 2017
@sjrd
Copy link
Collaborator

sjrd commented Aug 7, 2017

Ping @densh. We'll need this to be able to merge 0.6.x into master in the Scala.js repo.

@densh
Copy link
Collaborator

densh commented Aug 7, 2017

LGTM, thanks @xuwei-k ! 👍

@densh densh merged commit 04cbbb4 into portable-scala:master Aug 7, 2017
@densh
Copy link
Collaborator

densh commented Aug 7, 2017

@sjrd Yep, thanks for the heads up!

@xuwei-k xuwei-k deleted the sbt-1.0.0-RC2 branch August 7, 2017 11:24
@sjrd
Copy link
Collaborator

sjrd commented Aug 8, 2017

Thanks! Could I also get a release, please?

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

3 participants