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

Removed not used sbt-scala-native plugin and fixed importing the project into intellij #2382

Merged
merged 2 commits into from
Oct 12, 2018

Conversation

LukaszMarchewka
Copy link
Contributor

@LukaszMarchewka LukaszMarchewka commented Aug 11, 2018

I have had a problem during importing the project into intellij (version 2018.2) . Intellij tried to download scala-native for scala 2.12. I have solved the problem by:

  • removed not used scala-native-plugin
  • set fixed scala in version 2.11 for a native kernel

@johnynek
Copy link
Contributor

Why do you say it is a dead line?

Looks to me like support for scala native in the kernel module.

@ghost
Copy link

ghost commented Aug 11, 2018

Wow....this is work in progress for native, only recently added. More work is in the pipeline with new release of of sbt-crossproject like 2 days ago

@@ -16,4 +16,3 @@ addSbtPlugin("org.tpolecat" % "tut-plugin" % "0.6.6")
addSbtPlugin("org.portable-scala" % "sbt-scalajs-crossproject" % "0.5.0")
addSbtPlugin("org.portable-scala" % "sbt-scala-native-crossproject" % "0.5.0")
addSbtPlugin("org.scala-js" % "sbt-scalajs" % "0.6.24")
addSbtPlugin("org.scala-native" % "sbt-scala-native" % "0.3.7")
Copy link

Choose a reason for hiding this comment

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

This is OK, please add back

build.sbt Outdated
lazy val kernelNative = kernel.native.settings(
scalaVersion := "2.11.12",
crossScalaVersions := Seq("2.11.12")
)
Copy link

Choose a reason for hiding this comment

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

These settings (or any, for that matter) should not go directly on the internal project. Instead:

  • Add to a lazy val commonNativeSettings, as per js and jvm
  • then add the settings to the project using .nativeSettings(commonNativeSettings)

Apologies for any typos, code not tested ...

Ideally, we need dwijnand/sbt-travisci#11 here

@ghost
Copy link

ghost commented Aug 15, 2018

I have a PR for #2364 waiting to go. We could expedite this by merging as-is, and my new PR can tweak afterwards.

@codecov-io
Copy link

codecov-io commented Aug 15, 2018

Codecov Report

Merging #2382 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2382   +/-   ##
======================================
  Coverage    95.2%   95.2%           
======================================
  Files         361     361           
  Lines        6584    6584           
  Branches      282     282           
======================================
  Hits         6268    6268           
  Misses        316     316

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c56c4b...1a8bd84. Read the comment docs.

@LukaszMarchewka
Copy link
Contributor Author

@BennyHill sorry for delay, I wasn't able to update this faster. I have applied all your suggestions.

@ghost
Copy link

ghost commented Aug 15, 2018

@LukaszMarchewka Thanks!

You'll see in next PR for composite projects that the lines

lazy val kernelJVM = kernel.jvm
lazy val kernelJS = kernel.js
lazy val kernelNative = kernel.native

and similar will all disappear...so I'm not just being picky here 😄

@LukaJCB
Copy link
Member

LukaJCB commented Sep 22, 2018

@kailuowang Should we merge this for now, as it seems to resolve a common blocker for people wanting to contribute to cats?

LukaJCB
LukaJCB previously approved these changes Sep 22, 2018
easel
easel previously approved these changes Sep 25, 2018
Copy link
Contributor

@easel easel left a comment

Choose a reason for hiding this comment

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

LGTM. Intellij compatibility is good and so is native support. Might be worth changing the title to "Resolve scala-native build configuration issues" or something similar.

@Avasil
Copy link
Contributor

Avasil commented Oct 11, 2018

@LukaJCB @kailuowang any progress? I actually know about some potential contributors that are blocked by this :P

kailuowang
kailuowang previously approved these changes Oct 12, 2018
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

sorry about the delay, I missed this.

@kailuowang kailuowang dismissed stale reviews from easel, LukaJCB, and themself via 1a8bd84 October 12, 2018 01:04
@kailuowang kailuowang merged commit 6d1c167 into typelevel:master Oct 12, 2018
@kailuowang kailuowang added this to the 1.5 milestone Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants