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

drive-relative paths in Windows SHELL environments #2516

Merged
merged 11 commits into from
Nov 24, 2023
Merged

drive-relative paths in Windows SHELL environments #2516

merged 11 commits into from
Nov 24, 2023

Conversation

philwalk
Copy link
Contributor

@philwalk philwalk commented Nov 8, 2023

Most of this fix is a side-effect of updating os-lib to version 0.9.2, recently released.

The bug manifests itself in Windows shell environments, such as CYGWIN64, MSYS64, GIT-BASH, MINGW64, etc.
See os-lib#201 for a detailed description of the symptoms and cause.

A one-line change in InstallCompletions.scala. generalizes splitting of filesystem paths. The Windows jvm supports filesystem paths with either forward or back slashes, so splitting a path on File.separator can sometimes fail.

The same generalization (splitting filesystem paths) was also applied to InstallCompletions.scala.

A pattern was added to .gitignore to prevent vim swap files from cluttering the output of git status, or from accidentally being added. Let me know if I should instead configure this in my local environment and I'll revert it.

@Gedochao Gedochao changed the title fix for #2359, drive-relative paths in Windows SHELL environments drive-relative paths in Windows SHELL environments Nov 8, 2023
@Gedochao
Copy link
Contributor

Gedochao commented Nov 8, 2023

Hey, thanks for the contribution!
Do you think you could add an anti-regression test for this?
We do have integration tests running on Windows among other platforms.

@Gedochao
Copy link
Contributor

Gedochao commented Nov 8, 2023

On another note, you might need to rebase before we run the CI, some recent fixes may be necessary for it to pass anyway.

project/deps.sc Outdated Show resolved Hide resolved
@philwalk
Copy link
Contributor Author

philwalk commented Nov 8, 2023

On another note, you might need to rebase before we run the CI, some recent fixes may be necessary for it to pass anyway.

I have a working test case and am ready to push a commit.

Should I merge upstream/main into this branch before I push my next commit? Or does git pull --rebase do what we want? Or something else?

Here are the command lines I'm considering, feedback is welcome:

git rebase -i upstream/main fix-for-windows-shell-environments-issue-2359

Or, perhaps this:

git pull --rebase

Afterwards, I assume I do a git push, or do I want git push -f?

@Gedochao
Copy link
Contributor

Gedochao commented Nov 9, 2023

If you're okay with a squash merge of this PR, it's okay to merge upstream.
Rebase would only be necessary if you wanted to keep your commit structure after the PR is merged (since we don't want upstream merge commit in the git tree on the main branch).
push -f is only necessary for when you rewrite your commit history, which would only happen for a rebase.
So no -f necessary if you're just merging upstream.

TL;DR your PR deals with one thing, so it should be okay to merge upstream now and squash later (the squash being performed by the person who merges your PR, so probably me).

When in doubt, there's more guidelines on how to organise your PRs in our CONTRIBUTING doc here: https://github.com/VirtusLab/scala-cli/blob/main/CONTRIBUTING.md#rules-for-a-well-formed-pr

@philwalk
Copy link
Contributor Author

philwalk commented Nov 9, 2023

Added a test to verify that changes implemented in os-lib 0.9.2 are present.

@philwalk
Copy link
Contributor Author

philwalk commented Nov 9, 2023

It seems that one of the test failures may be related to the change in this file:

PackageTestDefinitions.scala

Splitting a path on File.separator is almost always a bug, although in this case I don't have enough context to confirm what the root cause of the failure is, so perhaps we should revert this change. AFAICT, the purpose of this code is to convert classpaths to a sequence of bare jarfile base names without the preceding path.

The problem with the code as it was is that in Windows, a classpath is legal with either forward slash, backslash, or both.
I tend to define classpath with forward slash.

I can't easily run the failing tests, but I just pushed a commit that addresses this.

I verified the new code with this scala 3 script, which lists classpath jar file basenames:

#!/usr/bin/env -S scala

import java.io.File

def main(args: Array[String]): Unit =
  val scalaLibCp = sys.props("java.class.path")
  val regex = "[\\\\/]+"
  val scalaLibJarNames: Array[String] = scalaLibCp.split(File.pathSeparator).map { entry =>
    entry.split(regex).last
  }
  for (jar <- scalaLibJarNames){
    printf("%s\n", jar)
  }

The purpose of the similar change in file InstallCompletions.scala is because the value of the SHELL environment variable can also have either forward slash or backslash paths in Windows. The previous code crashes in these environments (file not found, as I recall).

Some sample values in different environments:

Linux 5.15.0-78-generic x86_64 GNU/Linux : /bin/bash
MINGW64_NT-10.0-22621 3.4.9.x86_64 Msys: /bin/bash
CYGWIN_NT-10.0-22621 3.4.9-1.x86_64 Cygwin: /bin/bash

Although SHELL isn't defined in my Windows environments (CMD and powersh), it seems prudent to allow for it to have a path with backslashes.

@Gedochao
Copy link
Contributor

The scalafix check is also failing on the CI, might want to run ./mill -i __.fix before pushing changes.

@philwalk
Copy link
Contributor Author

philwalk commented Nov 10, 2023

The scalafix check is also failing on the CI, might want to run ./mill -i __.fix before pushing changes.

Thanks for the detailed feedback! Although I'm not able to get ./mill -i __fix to run in Windows (it crashes), I can run it in Ubuntu. I'll see about implementing the tests you describe. The purpose of the existing test was, as you pointed out, to insure that no regression has occurred in os-lib, although it also verifies that a newer version of os-lib is used in test setup.

I notice there are various places where tests are modified or disabled based on !Properties.isWin, I wonder if this PR allows some simplification. (I'm not proposing we attempt to do so as part of this PR, of course).

BTW, when I ran ./mill -i __.fix the first time, it ran to completion, removing two unneeded imports. Now that I have additional tests in scala.cli.integration.RunScriptTestDefinitions, it fails with the message below, so I deleted the out directory and will redo from scratch. Is there a shortcut when this happens?

$ ./mill -i __.fix
[51/1375] build-macros.fix
Rewriting and linting 3 Scala sources against 3 rules
error: SemanticDB not found: modules/build-macros/src/main/scala/scala/build/EitherCps.scala
error: SemanticDB not found: modules/build-macros/src/main/scala/scala/build/Ops.scala
error: SemanticDB not found: modules/build-macros/src/main/scala/scala/build/EitherSequence.scala
1 targets failed
build-macros.fix A semantic rewrite was run on a source file that has no associated META-INF/semanticdb/.../*.semanticdb

@philwalk
Copy link
Contributor Author

philwalk commented Nov 12, 2023

I ran integration.test.jvm to completion with some errors in Windows. It took 140 minutes. Here's the tail end of the log output:

scala.cli.integration.VersionTests:
  + version command 7.224s
Could not remove C:\Users\philwalk\workspace\scala-cli-winfix\out\integration\tmpDirBase.dest\working-dir\run-1729240687, ignoring it.
1 targets failed
integration.test.test 78 tests failed:
  scala.cli.integration.ConfigTests.repository credentials scala.cli.integration.ConfigTests.repository credentials
  scala.cli.integration.MarkdownTests.run a simple .md file with multiple using directives spread across scala script snippets scala.cli.integration.MarkdownTests.run a simple .md file with multiple using directives spread across scala script snippets
  scala.cli.integration.MarkdownTests.run a simple .md file with multiple using directives spread across scala raw snippets scala.cli.integration.MarkdownTests.run a simple .md file with multiple using directives spread across scala raw snippets
  scala.cli.integration.NativePackagerTests.building msi package scala.cli.integration.NativePackagerTests.building msi package
  scala.cli.integration.NativePackagerTests.building docker image scala.cli.integration.NativePackagerTests.building docker image
  and 73 more ...

real    140m6.267s
user    0m6.825s
sys     0m11.247s

I'm guessing I don't have all the required build tools installed.
It did pass the JAVA_HOME tests:

+ verify drive-relative JAVA_HOME works 4.336s
+ verify drive-relative JAVA_HOME works 4.386s

@Gedochao
Copy link
Contributor

BTW, when I ran ./mill -i __.fix the first time, it ran to completion, removing two unneeded imports. Now that I have additional tests in scala.cli.integration.RunScriptTestDefinitions, it fails with the message below, so I deleted the out directory and will redo from scratch. Is there a shortcut when this happens?

$ ./mill -i __.fix
[51/1375] build-macros.fix
Rewriting and linting 3 Scala sources against 3 rules
error: SemanticDB not found: modules/build-macros/src/main/scala/scala/build/EitherCps.scala
error: SemanticDB not found: modules/build-macros/src/main/scala/scala/build/Ops.scala
error: SemanticDB not found: modules/build-macros/src/main/scala/scala/build/EitherSequence.scala
1 targets failed
build-macros.fix A semantic rewrite was run on a source file that has no associated META-INF/semanticdb/.../*.semanticdb

Yeah, it's a known problem. We should probably look into fixing this.

Is there a shortcut when this happens?

As a workaround just run ./mill clean to get rid of the troublemaking semanticdb files between__.fix runs (either that or remove them manually yourself)

@Gedochao
Copy link
Contributor

It took 140 minutes

Hey, remember you can filter test suites. It's okay to only run the relevant ones.
https://github.com/VirtusLab/scala-cli/blob/main/DEV.md#run-integration-tests-with-the-jvm-launcher

So in your case:

./mill integration.test.jvm 'scala.cli.integration.RunTestsDefault.verify os.Path attributes'

@Gedochao
Copy link
Contributor

The CI seems to be failing on Windows.
Are your tests passing on your local Windows machine?

./mill integration.test.native 'scala.cli.integration.RunTestsDefault.verify drive-relative JAVA_HOME works'

@philwalk
Copy link
Contributor Author

philwalk commented Nov 14, 2023

Are your tests passing on your local Windows machine?

Yes, here is the output:

$ mymill integration.test.native 'scala.cli.integration.RunTestsDefault.verify drive-relative JAVA_HOME works'
Publishing Artifact(org.virtuslab.scala-cli,runner_3,0.0.1-SNAPSHOT) to ivy repo C:\Users\philwalk\workspace\scala-cli-winfix\out\repo\0.0.1-SNAPSHOT
Publishing Artifact(org.virtuslab.scala-cli,test-runner_3,0.0.1-SNAPSHOT) to ivy repo C:\Users\philwalk\workspace\scala-cli-winfix\out\repo\0.0.1-SNAPSHOT
Publishing Artifact(org.virtuslab.scala-cli,runner_2.13,0.0.1-SNAPSHOT) to ivy repo C:\Users\philwalk\workspace\scala-cli-winfix\out\repo\0.0.1-SNAPSHOT
Publishing Artifact(org.virtuslab.scala-cli,test-runner_2.13,0.0.1-SNAPSHOT) to ivy repo C:\Users\philwalk\workspace\scala-cli-winfix\out\repo\0.0.1-SNAPSHOT
Publishing Artifact(org.virtuslab.scala-cli,runner_2.12,0.0.1-SNAPSHOT) to ivy repo C:\Users\philwalk\workspace\scala-cli-winfix\out\repo\0.0.1-SNAPSHOT
Publishing Artifact(org.virtuslab.scala-cli,test-runner_2.12,0.0.1-SNAPSHOT) to ivy repo C:\Users\philwalk\workspace\scala-cli-winfix\out\repo\0.0.1-SNAPSHOT
scala.cli.integration.BloopTests:
scala.cli.integration.BspTests212:
scala.cli.integration.BspTests213:
scala.cli.integration.BspTestsDefault:
scala.cli.integration.CleanTests:
scala.cli.integration.CompileTests212:
scala.cli.integration.CompileTests213:
scala.cli.integration.CompileTestsDefault:
scala.cli.integration.CompleteTests:
scala.cli.integration.ConfigTests:
scala.cli.integration.DefaultFileTests:
scala.cli.integration.DefaultTests:
scala.cli.integration.DependencyUpdateTests:
scala.cli.integration.DirectoriesTests:
scala.cli.integration.DocTests212:
scala.cli.integration.DocTests213:
scala.cli.integration.DocTestsDefault:
scala.cli.integration.ExportJsonTestsDefault:
scala.cli.integration.ExportMillTests212:
scala.cli.integration.ExportMillTests213:
scala.cli.integration.ExportMillTestsDefault:
scala.cli.integration.ExportSbtTests212:
scala.cli.integration.ExportSbtTests213:
scala.cli.integration.ExportSbtTestsDefault:
scala.cli.integration.FixTests:
scala.cli.integration.FmtTests:
scala.cli.integration.GitHubTests:
scala.cli.integration.HadoopTests:
scala.cli.integration.HelpTests:
scala.cli.integration.InstallAndUninstallCompletionsTests:
scala.cli.integration.InstallHomeTests:
scala.cli.integration.JmhTests:
scala.cli.integration.LoggingTests:
scala.cli.integration.MarkdownTests:
scala.cli.integration.MetaCheck:
scala.cli.integration.NativePackagerTests:
scala.cli.integration.NewTests:
scala.cli.integration.PackageTests212:
scala.cli.integration.PackageTests213:
scala.cli.integration.PackageTestsDefault:
scala.cli.integration.PgpTests:
scala.cli.integration.PublishLocalTests212:
scala.cli.integration.PublishLocalTests213:
scala.cli.integration.PublishLocalTestsDefault:
scala.cli.integration.PublishSetupTests:
scala.cli.integration.PublishTests212:
scala.cli.integration.PublishTests213:
scala.cli.integration.PublishTestsDefault:
scala.cli.integration.ReplTests:
scala.cli.integration.ReplTests212:
scala.cli.integration.ReplTests213:
scala.cli.integration.ReplTestsDefault:
scala.cli.integration.RunTests212:
scala.cli.integration.RunTests213:
Running warmup test…
Compiling project (Scala 3.3.1, JVM (19))
Compiled project (Scala 3.3.1, JVM (19))
Done running warmup test.
Compiling project (Scala 3.3.1, JVM (8))
Compiled project (Scala 3.3.1, JVM (8))
scala.cli.integration.RunTestsDefault:
  + verify drive-relative JAVA_HOME works 8.501s
scala.cli.integration.ScriptWrapperTests:
scala.cli.integration.SharedRunTests:
scala.cli.integration.SipScalaTests:
scala.cli.integration.SparkTests212:
scala.cli.integration.SparkTests213:
scala.cli.integration.StandaloneLauncherTests:
scala.cli.integration.TestNativeImageOnScala3:
scala.cli.integration.TestTests212:
scala.cli.integration.TestTests213:
scala.cli.integration.TestTestsDefault:
scala.cli.integration.UpdateTests:
scala.cli.integration.VersionTests:

When I run all tests, I get some failures, but I probably don't have all build system pre-requisites installed (are they documented somewhere?) Here are the failing tests:

==> X scala.cli.integration.ConfigTests.repository credentials  11.255s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.MarkdownTests.run a simple .md file with multiple using directives spread across scala script snippets  4.039s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.MarkdownTests.run a simple .md file with multiple using directives spread across scala raw snippets  3.847s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.ReplTests212.ammonite scalapy  3.83s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.ReplTests213.ammonite scalapy  3.609s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.ReplTestsDefault.ammonite scalapy  4.255s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.RunTests212.Python and Scala sources  4.018s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.RunTests212.warn about transitive `using file` directive  3.297s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.RunTests213.Python and Scala sources  3.96s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.RunTests213.warn about transitive `using file` directive  3.413s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.RunTestsDefault.Python and Scala sources  3.899s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.RunTestsDefault.should throw exception for code compiled by scala 3.1.3  3.849s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.RunTestsDefault.warn about transitive `using file` directive  3.379s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.SparkTests212.package spark 2.4  93.91s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.SparkTests212.package spark 3.0  101.062s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.TestTests212.failing test native  29.38s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.TestTests212.Fail if no tests were run Native  30.76s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.TestTests213.failing test native  29.73s java.lang.AssertionError: assertion failed
==> X scala.cli.integration.TestTests213.Fail if no tests were run Native  29.022s java.lang.AssertionError: assertion failed

@philwalk

This comment was marked as off-topic.

…x-for-windows-shell-environments-issue-2359
@philwalk
Copy link
Contributor Author

philwalk commented Nov 20, 2023

I get a failure of the cli.base-image.nativeImage test, but only when I use mill.bat.

It succeeds if I launch it with the ./mill script:

 [1/7] Initializing...                                                                                   (17.9s @ 0.81GB)
  Version info: 'GraalVM 22.3.1 Java 17 CE'
  Java version info: '17.0.6+10-jvmci-22.3-b13'
  C compiler: cl.exe (microsoft, x64, 19.38.33130)
  Garbage collector: Serial GC
  4 user-specific feature(s)
  - com.oracle.svm.polyglot.scala.ScalaFeature
  - com.oracle.svm.thirdparty.gson.GsonFeature
  - scala.cli.internal.CsJniUtilsFeature
  - scala.cli.internal.LibsodiumjniFeature
 [2/7] Performing analysis...  [**********]                                                              (74.2s @ 3.85GB)
   27,029 (94.42%) of 28,626 classes reachable
   39,025 (71.85%) of 54,318 fields reachable
  130,869 (55.57%) of 235,512 methods reachable
      619 classes, 1,549 fields, and 4,551 methods registered for reflection
       85 classes,    82 fields, and    70 methods registered for JNI access
        8 native libraries: Advapi32, crypt32, ncrypt, ole32, psapi, shell32, version, winhttp
 [3/7] Building universe...                                                                               (6.6s @ 4.65GB)
 [4/7] Parsing methods...      [***]                                                                      (5.3s @ 3.83GB)
 [5/7] Inlining methods...     [****]                                                                     (2.2s @ 3.77GB)
 [6/7] Compiling methods...    [*****]                                                                   (30.8s @ 2.78GB)
 [7/7] Creating image...                                                                                  (6.1s @ 2.69GB)
   49.41MB (51.91%) for code area:    80,959 compilation units
   44.95MB (47.23%) for image heap:  450,671 objects and 207 resources
  837.02KB ( 0.86%) for other data
   95.18MB in total
 ---------------------------------------------------------------------------------------------------------
 Top 10 packages in code area:                               Top 10 object types in image heap:
    1.86MB scala.build.preprocessing.directives                10.27MB byte[] for code metadata
    1.72MB scala.cli.commands.shared                            6.88MB java.lang.Class
    1.48MB sun.security.ssl                                     5.91MB byte[] for java.lang.String
    1.12MB java.util                                            4.16MB java.lang.String
    1.02MB scala.collection.immutable                           3.32MB byte[] for general heap data
  989.44KB scala.build                                          2.64MB byte[] for embedded resources
  887.68KB scala.build.options                                  2.27MB com.oracle.svm.core.hub.DynamicHubCompanion
  832.95KB java.lang.invoke                                     1.39MB byte[] for reflection metadata
  820.28KB scala.cli.commands.publish                        1009.53KB c.o.svm.core.hub.DynamicHub$ReflectionMetadata
  718.19KB coursier.core                                      875.79KB java.lang.String[]
   37.45MB for 670 more packages                                5.97MB for 6476 more object types
 ---------------------------------------------------------------------------------------------------------
                         7.0s (4.6% of total time) in 50 GCs | Peak RSS: 7.40GB | CPU load: 6.09
 ---------------------------------------------------------------------------------------------------------
 Produced artifacts:
  C:\Users\philwalk\workspace\scala-cli-winfix\out\cli\base-image\nativeImage.dest\scala-cli.build_artifacts.txt (txt)
  C:\Users\philwalk\workspace\scala-cli-winfix\out\cli\base-image\nativeImage.dest\scala-cli.exe (executable)
 =========================================================
 Finished generating 'scala-cli' in 2m 30s.

but it fails if I launch it with mill.bat:

[1/7] Initializing...                                                                                   (17.5s @ 0.83GB)
 Version info: 'GraalVM 22.3.1 Java 17 CE'
 Java version info: '17.0.6+10-jvmci-22.3-b13'
 C compiler: cl.exe (microsoft, x64, 19.38.33130)
 Garbage collector: Serial GC
 4 user-specific feature(s)
 - com.oracle.svm.polyglot.scala.ScalaFeature
 - com.oracle.svm.thirdparty.gson.GsonFeature
 - scala.cli.internal.CsJniUtilsFeature
 - scala.cli.internal.LibsodiumjniFeature

Fatal error: java.lang.IllegalAccessError: class scala.cli.internal.CsJniUtilsFeature (in unnamed module @0x3ba97962) cannot access class com.oracle.svm.core.jdk.NativeLibrarySupport (in module org.graalvm.nativeimage.builder) because module org.graalvm.nativeimage.builder does not export com.oracle.svm.core.jdk to unnamed module @0x3ba97962
at scala.cli.internal.CsJniUtilsFeature.beforeAnalysis(CsJniUtilsFeature.java:18)
at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGenerator.lambda$runPointsToAnalysis$9(NativeImageGenerator.java:736)
at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.FeatureHandler.forEachFeature(FeatureHandler.java:85)
at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGenerator.runPointsToAnalysis(NativeImageGenerator.java:736)
at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGenerator.doRun(NativeImageGenerator.java:578)
at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGenerator.run(NativeImageGenerator.java:535)
at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGeneratorRunner.buildImage(NativeImageGeneratorRunner.java:403)
at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGeneratorRunner.build(NativeImageGeneratorRunner.java:580)
at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGeneratorRunner.main(NativeImageGeneratorRunner.java:128)
..............................................................................................................................................................................
0.7s (3.7% of total time) in 14 GCs | Peak RSS: 1.51GB | CPU load: 2.24
..............................................................................................................................................................................
Failed generating 'scala-cli' after 17.7s.
Error: Image build request failed with exit status 1
1 targets failed
cli.base-image.nativeImage os.SubprocessException: Result of cmd□: 1

os.proc.call(ProcessOps.scala:89)
io.github.alexarchambault.millnativeimage.NativeImage.$anonfun$nativeImage$8(NativeImage.scala:219)
mill.define.Task$TraverseCtx.evaluate(Task.scala:71)

It would be nice to get os-lib to log the actual command line that's failing, but I haven't figured that out yet.

@Gedochao
Copy link
Contributor

I get a failure of the cli.base-image.nativeImage test, but only when I use mill.bat.

It succeeds if I launch it with the ./mill script:

but it fails if I launch it with mill.bat:

There might be an issue with mill.bat 🤔 But that'd be a separate thing.
The CI is using ./mill, let's focus on making the test pass on CI.

It would be nice to get os-lib to log the actual command line that's failing, but I haven't figured that out yet.

Yeah, we don't seem to be getting the proper stacktrace on the CI. No idea if it's tied to os-lib at all, since it's only your test that's failing.
We can try logging some more to figure out why it doesn't work.

For now I merged the current main branch to yours to get the recent fixes, I have a suspicion the recent JAVA_HOME fix for running Bloop might help. If not, I'll try investigating more.

Can you replicate the failure we're getting on the CI on your local Windows:

./mill integration.test.native 'scala.cli.integration.RunTestsDefault.verify drive-relative JAVA_HOME works'

?
And if so, can you share the full output?
I don't have a Windows machine to test it locally at hand, but I can try something later this week.

@Gedochao
Copy link
Contributor

This is how it seems to fail on the CI.

Exception in thread "main" java.io.IOException: Cannot run program "D:\Users\runneradmin\AppData\Local\Coursier\cache\arc\https\cdn.azul.com\zulu\bin\zulu8.74.0.17-ca-jdk8.0.392-win_x64.zip\zulu8.74.0.17-ca-jdk8.0.392-win_x64\bin\java.exe" (in directory "D:\a\scala-cli\scala-cli\out\integration\tmpDirBase.dest\working-dir\run-895163350\test-192"): CreateProcess error=2, The system cannot find the file specified
	at [email protected]/java.lang.ProcessBuilder.start(ProcessBuilder.java:1143)
	at [email protected]/java.lang.ProcessBuilder.start(ProcessBuilder.java:1073)
	at os.proc.proc$lzyINIT1$1(ProcessOps.scala:123)
	at os.proc.proc$1(ProcessOps.scala:127)
	at os.proc.spawn(ProcessOps.scala:129)
	at os.proc.call(ProcessOps.scala:87)
	at scala.build.internal.OsLibc$.javaVersion(OsLibc.scala:80)
	at scala.build.internal.OsLibc$.javaHomeVersion(OsLibc.scala:95)
	at scala.cli.commands.shared.SharedOptions.$anonfun$33$$anonfun$1(SharedOptions.scala:532)
	at scala.Option.map(Option.scala:242)
	at scala.cli.commands.shared.SharedOptions.$anonfun$33(SharedOptions.scala:536)
	at scala.Option.orElse(Option.scala:477)
	at scala.cli.commands.shared.SharedOptions.bloopRifleConfig$$anonfun$1(SharedOptions.scala:537)
	at scala.build.EitherCps$Helper.apply(EitherCps.scala:19)
	at scala.cli.commands.shared.SharedOptions.bloopRifleConfig(SharedOptions.scala:547)
	at scala.cli.commands.shared.SharedOptions.compilerMaker(SharedOptions.scala:556)
	at scala.cli.commands.run.Run$.runCommand(Run.scala:145)
	at scala.cli.commands.shebang.Shebang$.runCommand(Shebang.scala:33)
	at scala.cli.commands.shebang.Shebang$.runCommand(Shebang.scala:26)
	at scala.cli.commands.ScalaCommand.run(ScalaCommand.scala:368)
	at scala.cli.commands.ScalaCommand.run(ScalaCommand.scala:350)
	at caseapp.core.app.CaseApp.main(CaseApp.scala:157)
	at scala.cli.commands.ScalaCommand.main(ScalaCommand.scala:335)
	at caseapp.core.app.CommandsEntryPoint.main(CommandsEntryPoint.scala:166)
	at scala.cli.ScalaCliCommands.main(ScalaCliCommands.scala:125)
	at scala.cli.ScalaCli$.main0(ScalaCli.scala:269)
	at scala.cli.ScalaCli$.main(ScalaCli.scala:108)
	at scala.cli.ScalaCli.main(ScalaCli.scala)
Caused by: java.io.IOException: CreateProcess error=2, The system cannot find the file specified
	at com.oracle.svm.core.jni.functions.JNIFunctions$NewObjectWithObjectArrayArgFunctionPointer.invoke(JNIFunctions.java)
	at com.oracle.svm.core.jni.functions.JNIFunctions.ThrowNew(JNIFunctions.java:882)
	at [email protected]/java.lang.ProcessImpl.create(ProcessImpl.java)
	at [email protected]/java.lang.ProcessImpl.<init>(ProcessImpl.java:499)
	at [email protected]/java.lang.ProcessImpl.start(ProcessImpl.java:158)
	at [email protected]/java.lang.ProcessBuilder.start(ProcessBuilder.java:1110)
	... 27 more

We'll try to investigate some more on a local machine later this week.

@philwalk
Copy link
Contributor Author

philwalk commented Nov 21, 2023

This is how it seems to fail on the CI.

I fixed the probable cause for this error, I'll push what I have after a review and some cleanup.

I found some other occasional failures, like this one (if jvm directory is already present, it fails):

diff --git a/.github/scripts/generate-native-image.sh b/.github/scripts/generate-native-image.sh
-  ./mill.bat -i ci.copyJvm --dest jvm
+  if [ ! -d jvm ]; then
+    ./mill.bat -i ci.copyJvm --dest jvm
+  fi

I also upgraded .mill-version to 0.11.5, which simplified some things in my environment.

For now I merged the current main branch to yours to get the recent fixes
I did a commit and then pull, resulting in a merge, I hope that's right.

Can you replicate the failure we're getting on the CI on your local Windows:

./mill integration.test.native 'scala.cli.integration.RunTestsDefault.verify drive-relative JAVA_HOME works'

It now fails, although it's different than what the CI is seeing.

The string millbuild.build$CliIntegration$IntegrationScalaTests$TestHelper@33caafd6 looks possibly like what happens when a path with backslashes is passed on a shell command line, the backslashes are effectively discarded. I've seen clear evidence of that here and there. I'll look at mill code to see if anything jumps out.

Here's the failure output:

``` ./mill clean ; ./mill integration.test.native 'scala.cli.integration.RunTestsDefault.verify drive-relative JAVA_HOME works; [compiling ...] [info] compiling 1 Scala source to C:\Users\philwalk\workspace\scala-cli-winfix\out\scala3-graal-processor\compile.dest\classes ... [info] done compiling Checking https://github.com/coursier/jvm-index/raw/master/index.json Checked https://github.com/coursier/jvm-index/raw/master/index.json Downloading https://github.com/coursier/jvm-index/raw/master/index.json Downloaded https://github.com/coursier/jvm-index/raw/master/index.json
C:\Users\philwalk\workspace\scala-cli-winfix>chcp 437 
Active code page: 437
**********************************************************************
** Visual Studio 2022 Developer Command Prompt v17.8.0
** Copyright (c) 2022 Microsoft Corporation
**********************************************************************
[vcvarsall.bat] Environment initialized for: 'x64'
========================================================================================================================
GraalVM Native Image: Generating 'scala-cli' (executable)...
========================================================================================================================
Warning: Feature class scala.cli.internal.CsJniUtilsFeature is annotated with the deprecated annotation @AutomaticFeature. Support for this annotation will be removed in a future version of GraalVM. Applications should register a feature using the option --features=scala.cli.internal.CsJniUtilsFeature
Warning: Feature class scala.cli.internal.LibsodiumjniFeature is annotated with the deprecated annotation @AutomaticFeature. Support for this annotation will be removed in a future version of GraalVM. Applications should register a feature using the option --features=scala.cli.internal.LibsodiumjniFeature
Warning: Could not resolve ch.epfl.scala.bsp4j.FileChangeType for reflection configuration. Reason: java.lang.ClassNotFoundException: ch.epfl.scala.bsp4j.FileChangeType.
Warning: Could not resolve ch.epfl.scala.bsp4j.TaskDataKind for reflection configuration. Reason: java.lang.ClassNotFoundException: ch.epfl.scala.bsp4j.TaskDataKind.
[1/7] Initializing...                                                                                   (17.9s @ 0.38GB)
 Version info: 'GraalVM 22.3.1 Java 17 CE'
 Java version info: '17.0.6+10-jvmci-22.3-b13'
 C compiler: cl.exe (microsoft, x64, 19.38.33130)
 Garbage collector: Serial GC
 4 user-specific feature(s)
 - com.oracle.svm.polyglot.scala.ScalaFeature
 - com.oracle.svm.thirdparty.gson.GsonFeature
 - scala.cli.internal.CsJniUtilsFeature
 - scala.cli.internal.LibsodiumjniFeature
[2/7] Performing analysis...  [**********]                                                              (75.0s @ 4.79GB)
  27,025 (94.42%) of 28,622 classes reachable
  39,039 (71.88%) of 54,315 fields reachable
 130,860 (55.57%) of 235,503 methods reachable
     619 classes, 1,549 fields, and 4,553 methods registered for reflection
      85 classes,    82 fields, and    70 methods registered for JNI access
       8 native libraries: Advapi32, crypt32, ncrypt, ole32, psapi, shell32, version, winhttp
[3/7] Building universe...                                                                               (6.4s @ 5.34GB)
[4/7] Parsing methods...      [***]                                                                      (6.3s @ 3.82GB)
[5/7] Inlining methods...     [****]                                                                     (2.1s @ 3.50GB)
[6/7] Compiling methods...    [*****]                                                                   (29.7s @ 3.50GB)
[7/7] Creating image...                                                                                  (6.2s @ 3.51GB)
  49.41MB (51.91%) for code area:    80,960 compilation units
  44.95MB (47.23%) for image heap:  450,659 objects and 207 resources
 839.84KB ( 0.86%) for other data
  95.18MB in total
------------------------------------------------------------------------------------------------------------------------
Top 10 packages in code area:                               Top 10 object types in image heap:
   1.85MB scala.build.preprocessing.directives                10.27MB byte[] for code metadata
   1.72MB scala.cli.commands.shared                            6.88MB java.lang.Class
   1.48MB sun.security.ssl                                     5.91MB byte[] for java.lang.String
   1.12MB java.util                                            4.16MB java.lang.String
   1.02MB scala.collection.immutable                           3.32MB byte[] for general heap data
 989.04KB scala.build                                          2.64MB byte[] for embedded resources
 887.08KB scala.build.options                                  2.27MB com.oracle.svm.core.hub.DynamicHubCompanion
 832.95KB java.lang.invoke                                     1.39MB byte[] for reflection metadata
 820.09KB scala.cli.commands.publish                        1009.38KB c.o.svm.core.hub.DynamicHub$ReflectionMetadata
 717.72KB coursier.core                                      875.65KB java.lang.String[]
  37.45MB for 670 more packages                                5.97MB for 6468 more object types
------------------------------------------------------------------------------------------------------------------------
                        6.5s (4.3% of total time) in 55 GCs | Peak RSS: 7.35GB | CPU load: 6.04
------------------------------------------------------------------------------------------------------------------------
Produced artifacts:
 C:\Users\philwalk\workspace\scala-cli-winfix\out\cli\base-image\nativeImage.dest\scala-cli.build_artifacts.txt (txt)
 C:\Users\philwalk\workspace\scala-cli-winfix\out\cli\base-image\nativeImage.dest\scala-cli.exe (executable)
========================================================================================================================

Finished generating 'scala-cli' in 2m 31s.
An unexpected error occurred

Exception in thread "MillServerActionRunner" java.lang.RuntimeException: Unknown ctx: millbuild.build$CliIntegration$IntegrationScalaTests$TestHelper@33caafd6
	at scala.sys.package$.error(package.scala:27)
	at mill.eval.GroupEvaluator$$anonfun$1.$anonfun$applyOrElse$6(GroupEvaluator.scala:119)
	at scala.collection.Iterator$UnfoldIterator.hasNext(Iterator.scala:1279)
	at scala.collection.mutable.Growable.addAll(Growable.scala:61)
	at scala.collection.mutable.Growable.addAll$(Growable.scala:57)
	at scala.collection.immutable.VectorBuilder.addAll(Vector.scala:1824)
	at scala.collection.immutable.Vector$.from(Vector.scala:1396)
	at scala.collection.immutable.Vector$.from(Vector.scala:34)
	at scala.collection.IterableFactory.unfold(Factory.scala:124)
	at scala.collection.IterableFactory.unfold$(Factory.scala:124)
	at scala.collection.immutable.Vector$.unfold(Vector.scala:34)
	at mill.eval.GroupEvaluator$$anonfun$1.applyOrElse(GroupEvaluator.scala:113)
	at mill.eval.GroupEvaluator$$anonfun$1.applyOrElse(GroupEvaluator.scala:89)
	at scala.collection.Iterator$$anon$7.hasNext(Iterator.scala:525)
	at scala.collection.Iterator$$anon$10.hasNext(Iterator.scala:601)
	at scala.collection.IterableOnceOps.foldLeft(IterableOnce.scala:675)
	at scala.collection.IterableOnceOps.foldLeft$(IterableOnce.scala:670)
	at scala.collection.AbstractIterator.foldLeft(Iterator.scala:1300)
	at scala.collection.IterableOnceOps.sum(IterableOnce.scala:945)
	at scala.collection.IterableOnceOps.sum$(IterableOnce.scala:943)
	at scala.collection.AbstractIterator.sum(Iterator.scala:1300)
	at mill.eval.GroupEvaluator.evaluateGroupCached(GroupEvaluator.scala:137)
	at mill.eval.GroupEvaluator.evaluateGroupCached$(GroupEvaluator.scala:44)
	at mill.eval.EvaluatorImpl.evaluateGroupCached(EvaluatorImpl.scala:15)
	at mill.eval.EvaluatorCore.$anonfun$evaluate0$2(EvaluatorCore.scala:116)
	at scala.concurrent.impl.Promise$Transformation.run(Promise.scala:467)
	at mill.eval.ExecutionContexts$RunNow$.execute(ExecutionContexts.scala:13)
	at scala.concurrent.impl.Promise$Transformation.submitWithValue(Promise.scala:429)
	at scala.concurrent.impl.Promise$DefaultPromise.submitWithValue(Promise.scala:338)
	at scala.concurrent.impl.Promise$DefaultPromise.dispatchOrAddCallbacks(Promise.scala:312)
	at scala.concurrent.impl.Promise$DefaultPromise.map(Promise.scala:182)
	at mill.eval.EvaluatorCore.$anonfun$evaluate0$1(EvaluatorCore.scala:92)
	at mill.eval.EvaluatorCore.$anonfun$evaluate0$1$adapted(EvaluatorCore.scala:90)
	at scala.collection.immutable.Vector.foreach(Vector.scala:2124)
	at mill.eval.EvaluatorCore.evaluate0(EvaluatorCore.scala:90)
	at mill.eval.EvaluatorCore.$anonfun$evaluate$1(EvaluatorCore.scala:43)
	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:59)
	at mill.eval.EvaluatorCore.evaluate(EvaluatorCore.scala:34)
	at mill.eval.EvaluatorCore.evaluate$(EvaluatorCore.scala:26)
	at mill.eval.EvaluatorImpl.evaluate(EvaluatorImpl.scala:15)
	at mill.main.RunScript$.evaluateNamed(RunScript.scala:38)
	at mill.main.RunScript$.$anonfun$evaluateTasksNamed$2(RunScript.scala:26)
	at scala.util.Either.map(Either.scala:382)
	at mill.main.RunScript$.evaluateTasksNamed(RunScript.scala:26)
	at mill.runner.MillBuildBootstrap$.evaluateWithWatches(MillBuildBootstrap.scala:399)
	at mill.runner.MillBuildBootstrap.$anonfun$processFinalTargets$3(MillBuildBootstrap.scala:308)
	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:59)
	at mill.runner.MillBuildBootstrap.processFinalTargets(MillBuildBootstrap.scala:308)
	at mill.runner.MillBuildBootstrap.evaluateRec(MillBuildBootstrap.scala:196)
	at mill.runner.MillBuildBootstrap.evaluate(MillBuildBootstrap.scala:48)
	at mill.runner.MillMain$.$anonfun$main0$6(MillMain.scala:234)
	at mill.runner.Watching$.watchLoop(Watching.scala:27)
	at mill.runner.MillMain$.$anonfun$main0$1(MillMain.scala:219)
	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:59)
	at scala.Console$.withErr(Console.scala:193)
	at mill.api.SystemStreams$.$anonfun$withStreams$2(SystemStreams.scala:62)
	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:59)
	at scala.Console$.withOut(Console.scala:164)
	at mill.api.SystemStreams$.$anonfun$withStreams$1(SystemStreams.scala:61)
	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:59)
	at scala.Console$.withIn(Console.scala:227)
	at mill.api.SystemStreams$.withStreams(SystemStreams.scala:60)
	at mill.runner.MillMain$.main0(MillMain.scala:101)
	at mill.runner.MillServerMain$.main0(MillServerMain.scala:83)
	at mill.runner.MillServerMain$.main0(MillServerMain.scala:35)
	at mill.runner.Server.$anonfun$handleRun$1(MillServerMain.scala:187)
	at java.base/java.lang.Thread.run(Thread.java:833)

I ran the mill server in an Idea debugger session, but the graalvm compile stage crashes in the same way that we're seeing when testing with mill.bat. Here's the crash:

[info] done compiling
Publishing Artifact(org.virtuslab.scala-cli,runner_3,1.0.7-SNAPSHOT) to ivy repo C:\Users\philwalk\workspace\scala-cli-winfix\out\repo\1.0.7-SNAPSHOT
Publishing Artifact(org.virtuslab.scala-cli,test-runner_3,1.0.7-SNAPSHOT) to ivy repo C:\Users\philwalk\workspace\scala-cli-winfix\out\repo\1.0.7-SNAPSHOT
Publishing Artifact(org.virtuslab.scala-cli,runner_2.13,1.0.7-SNAPSHOT) to ivy repo C:\Users\philwalk\workspace\scala-cli-winfix\out\repo\1.0.7-SNAPSHOT
Publishing Artifact(org.virtuslab.scala-cli,test-runner_2.13,1.0.7-SNAPSHOT) to ivy repo C:\Users\philwalk\workspace\scala-cli-winfix\out\repo\1.0.7-SNAPSHOT
Publishing Artifact(org.virtuslab.scala-cli,runner_2.12,1.0.7-SNAPSHOT) to ivy repo C:\Users\philwalk\workspace\scala-cli-winfix\out\repo\1.0.7-SNAPSHOT
Publishing Artifact(org.virtuslab.scala-cli,test-runner_2.12,1.0.7-SNAPSHOT) to ivy repo C:\Users\philwalk\workspace\scala-cli-winfix\out\repo\1.0.7-SNAPSHOT
Checking https://github.com/coursier/jvm-index/raw/master/index.json
Checked https://github.com/coursier/jvm-index/raw/master/index.json
Downloading https://github.com/coursier/jvm-index/raw/master/index.json
Downloaded https://github.com/coursier/jvm-index/raw/master/index.json
C:\Users\philwalk\workspace\scala-cli-winfix>chcp 437 
Active code page: 437
**********************************************************************
** Visual Studio 2022 Developer Command Prompt v17.8.0
** Copyright (c) 2022 Microsoft Corporation
**********************************************************************
[vcvarsall.bat] Environment initialized for: 'x64'
==============================================================================
GraalVM Native Image: Generating 'scala-cli' (executable)...
==============================================================================
Warning: Feature class scala.cli.internal.CsJniUtilsFeature is annotated with the deprecated annotation @AutomaticFeature. Support for this annotation will be removed in a future version of GraalVM. Applications should register a feature using the
option --features=scala.cli.internal.CsJniUtilsFeature

Warning: Feature class scala.cli.internal.LibsodiumjniFeature is annotated with the deprecated annotation @AutomaticFeature. Support for this annotation will be removed in a future version of GraalVM. Applications should register a feature using the option --features=scala.cli.internal.LibsodiumjniFeature

Warning: Could not resolve ch.epfl.scala.bsp4j.FileChangeType for reflection configuration. Reason: java.lang.ClassNotFoundException: ch.epfl.scala.bsp4j.FileChangeType.

Warning: Could not resolve ch.epfl.scala.bsp4j.TaskDataKind for reflection configuration. Reason: java.lang.ClassNotFoundException: ch.epfl.scala.bsp4j.TaskDataKind.


[1/7] Initializing... (21.4s @ 0.43GB)
Version info: 'GraalVM 22.3.1 Java 17 CE'
Java version info: '17.0.6+10-jvmci-22.3-b13'
C compiler: cl.exe (microsoft, x64, 19.38.33130)
Garbage collector: Serial GC
4 user-specific feature(s)

  • com.oracle.svm.polyglot.scala.ScalaFeature
  • com.oracle.svm.thirdparty.gson.GsonFeature
  • scala.cli.internal.CsJniUtilsFeature
  • scala.cli.internal.LibsodiumjniFeature
------------------------------------------------------------------------------------------------------------------------
                        0.7s (3.3% of total time) in 16 GCs | Peak RSS: 1.42GB | CPU load: 2.10
======================================================================

Failed generating 'scala-cli' after 21.5s.

Fatal error: java.lang.IllegalAccessError: class scala.cli.internal.CsJniUtilsFeature (in unnamed module @0x1ea96ff2) cannot access class com.oracle.svm.core.jdk.NativeLibrarySupport (in module org.graalvm.nativeimage.builder) because module org.graalvm.nativeimage.builder does not export com.oracle.svm.core.jdk to unnamed module @0x1ea96ff2
	at scala.cli.internal.CsJniUtilsFeature.beforeAnalysis(CsJniUtilsFeature.java:18)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGenerator.lambda$runPointsToAnalysis$9(NativeImageGenerator.java:736)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.FeatureHandler.forEachFeature(FeatureHandler.java:85)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGenerator.runPointsToAnalysis(NativeImageGenerator.java:736)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGenerator.doRun(NativeImageGenerator.java:578)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGenerator.run(NativeImageGenerator.java:535)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGeneratorRunner.buildImage(NativeImageGeneratorRunner.java:403)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGeneratorRunner.build(NativeImageGeneratorRunner.java:580)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGeneratorRunner.main(NativeImageGeneratorRunner.java:128)
Error: Image build request failed with exit status 1

@Gedochao
Copy link
Contributor

Gedochao commented Nov 22, 2023

I found some other occasional failures, like this one (if jvm directory is already present, it fails):

diff --git a/.github/scripts/generate-native-image.sh b/.github/scripts/generate-native-image.sh
-  ./mill.bat -i ci.copyJvm --dest jvm
+  if [ ! -d jvm ]; then
+    ./mill.bat -i ci.copyJvm --dest jvm
+  fi

I believe that is expected.

I also upgraded .mill-version to 0.11.5, which simplified some things in my environment.

We can't upgrade mill to 0.11.5 due to com-lihaoyi/mill#2844, unfortunately.
We tried and had to revert it in #2529

It has since been addressed in 0.11.6, but the upgrade seems to fail for now.
I haven't looked into why just yet. You can track the progress in #2572
Either way, please do not upgrade mill in this PR

EDIT: 0.11.6 seems to have its own share of issues, it seems, we'll probably wait for 0.11.7.
EDIT: Managed to make mill 0.11.6 work with scala-cli, the CI is green in #2572, can use that once it gets merged

Copy link
Contributor

@Gedochao Gedochao left a comment

Choose a reason for hiding this comment

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

Hey, I can see the CI is green already!
Great work on adapting the mill script for Windows.
You may be the first to work in this repository in a Windows environment this extensively 😅
I commented on some cosmetics, but it looks good already!
And thanks for the persistence at making this work!

mill Outdated Show resolved Hide resolved
mill Show resolved Hide resolved
Copy link
Contributor

@Gedochao Gedochao left a comment

Choose a reason for hiding this comment

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

LGTM

@Gedochao Gedochao merged commit 8df7956 into VirtusLab:main Nov 24, 2023
58 checks passed
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.

Error converting relative paths to absolute paths in Windows if relative path has leading slash or backslash
2 participants