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

Fix sbt plugin incompatibilities on Windows #2189

Merged

Conversation

WojciechMazur
Copy link
Contributor

This PR fixes fatal errors in the sbt plugin when trying to use it on Windows. Changes include:

  • detection of os used in sbt plugin
  • using where command instead of which
  • passing paths in process commands in sequences, to correctly handle spaces
  • fixes to regex and globing filters including usage of the correct path separator

Some methods with single usage were inlined.

There are still some pending sbt plugin related issues that would need to resolve in the following PRs, e.g.:

  • Handling llvm-config dependent settings - llvm-config is not included in prebuilt binaries targeted for Windows. We don't want to force users to build it locally. Probably for include and lib dir we can use fixed relative paths based on the location of clang or clang++. I have such change prepared, however I decided to not include it now, because of next problem
  • Adding proper handling and detection of include and lib directories, however might finally decide to leave this part for users to configure.

@ekrich
Copy link
Member

ekrich commented Mar 3, 2021

I had put in an issue #2127 and had at least a plan sketch to use the llvm-config command to get compiling and linking options and only adding defaults if it didn't work. Using Mac tools instead of brew is strange because there is no installed llvm-config for example.

I found that llvm-config as it is now didn't work as it normally is llvm-config-6 for example if version 6. I also found that clang++ is normally aliased to clang so maybe it would make more sense to default to CLANG_PATH instead of the current. Maybe there is a use case to have a different version of C and C++ compilers but I am not sure.

I think PR right now looks good.

@@ -185,4 +178,12 @@ private[scalanative] object NativeLib {
case true => Some(path)
case false => None
}

private def makeDirPath(path: Path, elems: String*): String = {
val pathSep = if (Platform.isWindows) raw"\\" else File.separator
Copy link
Member

Choose a reason for hiding this comment

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

This is needed because it is inside a glob which is like a regex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think that glob in java is also implemented based on regex.
Without this, we would not be able to correctly find all native libs.

@@ -85,7 +85,8 @@ object Discover {
private[scalanative] def checkClangVersion(pathToClangBinary: Path): Unit = {
def versionMajorFull(clang: String): (Int, String) = {
val versionCommand = s"$clang --version"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unused?

val path = Process(s"which $binaryNameOrPath")
val binaryNameOrPath = sys.env.getOrElse(envPath, binaryName)
val locateCmd = if (Platform.isWindows) "where" else "which"
val path = Process(s"$locateCmd $binaryNameOrPath")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this also use a Seq for the arguments?

package scala.scalanative.build

object Platform {
private lazy val osUsed = System.getProperty("os.name", "unknown").toLowerCase
Copy link
Collaborator

Choose a reason for hiding this comment

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

This won't work on a Turkish/Lithuanian/Azeri system.

Suggested change
private lazy val osUsed = System.getProperty("os.name", "unknown").toLowerCase
private lazy val osUsed = System.getProperty("os.name", "unknown").toLowerCase(Locale.ROOT)

- Use toLowerCase with Root locale
- Pass command as sequence instead of string
- Remove duplicated commands definitions
@WojciechMazur WojciechMazur requested a review from sjrd March 15, 2021 09:53
@sjrd sjrd merged commit fdc495d into scala-native:master Mar 15, 2021
ekrich pushed a commit to ekrich/scala-native that referenced this pull request May 21, 2021
This PR fixes fatal errors in the sbt plugin when trying to use it on Windows.
Changes include:

* detection of os used in sbt plugin
* using `where` command instead of `which` 
* passing paths in process commands in sequences, to correctly handle spaces
* fixes to regex and globing filters including usage of the correct path separator

Some methods with single usage were inlined.
WojciechMazur added a commit to WojciechMazur/scala-native that referenced this pull request Aug 25, 2021
This PR fixes fatal errors in the sbt plugin when trying to use it on Windows.
Changes include:

* detection of os used in sbt plugin
* using `where` command instead of `which` 
* passing paths in process commands in sequences, to correctly handle spaces
* fixes to regex and globing filters including usage of the correct path separator

Some methods with single usage were inlined.
@WojciechMazur WojciechMazur deleted the feature/sbt-plugin-windows-support branch December 24, 2021 05:18
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.

3 participants