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

Some fixes for the sbt build on Windows #4913

Merged
merged 4 commits into from
Jan 26, 2016

Conversation

szeiger
Copy link
Contributor

@szeiger szeiger commented Jan 21, 2016

I'm using MSYS on all my Windows machines. Do we have a Cygwin user who could review / test this?

The generated scripts for "quick" are not portable anyway (neither in
the sbt build nor in the ant build) because they contain absolute
paths. When using the sh versions on Windows in MSYS or Cygwin, the
classpath must be in Windows format because "java" expects it that way.
Note that none of this applies to "pack" (and thus to distribution
builds) where the classpath is constructed dynamically by the launcher
scripts itself.
@scala-jenkins scala-jenkins added this to the 2.11.8 milestone Jan 21, 2016
@lrytz
Copy link
Member

lrytz commented Jan 25, 2016

@szeiger i just tried with a fresh install of cygwin, jdk 1.8.0_71-b15, with the following result:

[info] Loading project definition from C:\Users\luc\Desktop\scala\project
C:\Users\luc\Desktop\scala\build.sbt:770: error: overloaded method value setExecutable with alternatives:
  (x$1: Boolean)Boolean <and>
  (x$1: Boolean,x$2: Boolean)Boolean
 cannot be applied to (Boolean, ownerOnly: Boolean)
      if(!f.getAbsoluteFile.setExecutable(true, ownerOnly = false))
                            ^
C:\Users\luc\Desktop\scala\build.sbt:772: error: overloaded method value setReadable with alternatives:
  (x$1: Boolean)Boolean <and>
  (x$1: Boolean,x$2: Boolean)Boolean
 cannot be applied to (Boolean, ownerOnly: Boolean)
      if(!f.getAbsoluteFile.setReadable(true, ownerOnly = false))
                            ^
sbt.compiler.EvalException: Type error in expression
        at sbt.compiler.Eval.checkError(Eval.scala:384)
        at sbt.compiler.Eval.compileAndLoad(Eval.scala:183)
        at sbt.compiler.Eval.evalCommon(Eval.scala:152)
...

@lrytz
Copy link
Member

lrytz commented Jan 25, 2016

not sure why the named argument works in some cases..

@szeiger
Copy link
Contributor Author

szeiger commented Jan 25, 2016

Hm, using named args with a Java library probably wasn't a good idea. AFAIR argument names are not in the bytecode (and the Java guys probably don't care about compatibility and will change them freely, too). I'll change it to use the uglier call syntax without named args.

@lrytz
Copy link
Member

lrytz commented Jan 25, 2016

I'm just surprised it worked on our jenkins build..

@lrytz
Copy link
Member

lrytz commented Jan 25, 2016

would be good if we could reproduce this and log ticket. related to #4735

Instead of waiting for a Java-8-only build to use
`Files.setPosixPermissions()` we can call `setExecutable` and
`setReadable`, both of which are available on Java 6.
@szeiger
Copy link
Contributor Author

szeiger commented Jan 25, 2016

Now the old version fails on my Windows machine, too, without any changes to the Java or sbt installation.

@szeiger
Copy link
Contributor Author

szeiger commented Jan 25, 2016

Oh, and the Jenkins build succeeded because #4884 hasn't been merged yet, so the sbt build is not tested on Jenkins.

@lrytz
Copy link
Member

lrytz commented Jan 25, 2016

now for sbt package i get:

[info] Packaging C:\Users\luc\Desktop\scala\build-sbt\pack\lib\scala-compiler.jar ...
[trace] Stack trace suppressed: run last compiler/compile:packageBin for the full output.
[error] (compiler/compile:packageBin) java.util.zip.ZipException: duplicate entry: repl-jline.properties
[error] Total time: 9 s, completed Jan 25, 2016 5:52:38 PM

@szeiger
Copy link
Contributor Author

szeiger commented Jan 25, 2016

@lrytz You need to run sbt dist/mkQuick

@lrytz
Copy link
Member

lrytz commented Jan 25, 2016

ok, that succeeded. then:

luc@lucwin /cygdrive/c/Users/luc/Desktop/scala
$ build-sbt/quick/bin/scala
build-sbt/quick/bin/scala: line 10: $'\r': command not found
build-sbt/quick/bin/scala: line 11: syntax error near unexpected token `$'{\r''
'uild-sbt/quick/bin/scala: line 11: `findScalaHome () {

luc@lucwin /cygdrive/c/Users/luc/Desktop/scala
$

@lrytz
Copy link
Member

lrytz commented Jan 25, 2016

@szeiger
Copy link
Contributor Author

szeiger commented Jan 25, 2016

@lrytz What does git config core.autocrlf say? ScalaTool doesn't perform any EOL normalization on the files it generates, so the batch and shell script templates need to have the correct EOLs already. Setting core.autocrlf to input (and doing a fresh checkout) should do the trick.

There are no changes in this regard by this PR. I'd expect the same behavior with or without it. We should make the EOL handling in ScalaTool more elegant to avoid this issue (but I doubt it will be the only one you encounter if you get CRLF EOLs into all files)

We now normalize EOLs in `ScalaTool` to LF when reading the templates,
and write them as CRLF or LF depending on the target platform. This
allows the script templates to be stored locally with either LF or CRLF
line endings (which can both happen, depending on the platform and git
configuration).
@lrytz
Copy link
Member

lrytz commented Jan 25, 2016

ok, i set core.autocrlf to input, did a fresh checkout, ran sbt dist/mkQuick, then i get:

luc@lucwin /cygdrive/c/Users/luc/Desktop/scala
$ build-sbt/quick/bin/scala
Error: Could not find or load main class scala.tools.nsc.MainGenericRunner

@lrytz
Copy link
Member

lrytz commented Jan 25, 2016

updated file in the gist: https://gist.github.com/lrytz/d28f793b3338b948e352

@szeiger
Copy link
Contributor Author

szeiger commented Jan 25, 2016

I've added another commit to make the EOL handling more resilient (in the sbt build; the ant build still depends on the correct format).

@lrytz, this looks like a problem with Cygwin's path handling. Can you prefix line 23 of the generated script with echo and send me the output?

@szeiger
Copy link
Contributor Author

szeiger commented Jan 25, 2016

Actually, it looks like the script was generated without this PR applied. Either that, or there's a problem with OS detection in org.apache.commons.lang3.SystemUtils. This should print true when Windows is detected in a similar way:

scala -e "val os = System.getProperty(\"os.name\"); println(os); println(os.startsWith(\"Windows\"))"

@lrytz
Copy link
Member

lrytz commented Jan 25, 2016

aah, sorry, i forgot to apply the PR after the second checkout..

@lrytz
Copy link
Member

lrytz commented Jan 25, 2016

ok, everything worked fine now, played a bit with scala and scalac

@lrytz
Copy link
Member

lrytz commented Jan 25, 2016

i guess the "integrate-ide" failure is spurious (@SethTisue ?)

@lrytz
Copy link
Member

lrytz commented Jan 25, 2016

LGTM!

i tried again without setting core.autocrlf to input, that now works as well.

@SethTisue
Copy link
Member

i guess the "integrate-ide" failure is spurious (@SethTisue ?)

yes, we may have a new problem with integrate-ide failing intermittently; I opened scala/scala-dev#76 on it

lrytz added a commit that referenced this pull request Jan 26, 2016
Some fixes for the sbt build on Windows
@lrytz lrytz merged commit 47b8910 into scala:2.11.x Jan 26, 2016
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.

4 participants