Fix bsp: getMillBuildClasspath return URI string #1325

camper42 · 2021-05-18T03:21:58Z

getMillBuildClasspath is expect to return URI, otherwise IDEA bsp import will have
java.lang.IllegalArgumentException: Missing scheme error on MacOS and
Java.net.URISyntaxException: Illegal character in opaque part at index 2: ... on Windows

getMillBuildClasspath is expect to return URI, otherwise IDEA bsp import will have `java.lang.IllegalArgumentException: Missing scheme` error on MacOS and `Java.net.URISyntaxException: Illegal character in opaque part at index 2: ...` on Windows

camper42 · 2021-05-18T03:25:10Z

problem I mentioned at #1285 (comment)
seems caused by https://github.com/com-lihaoyi/mill/pull/1285/files#diff-a8dcb229dc6fe3ad91d4667a94174520d2d13cf522738fed08023105ac955d00R165

Path.wrapped return java.nio.file.Path and toUri is missing

@lefou @ScalaWilliam pls review

lefou

As it seems this code isn't covered by any tests, I'd like to have at least one written "It works for me" from someone who really tested this locally, for each platform.

lefou · 2021-05-18T07:00:00Z

bsp/src/mill/bsp/ModuleUtils.scala

@@ -162,7 +162,7 @@ object ModuleUtils {
    val binarySource =
      if (sources) all.filter(url => isPathSourceJar(url))
      else all.filter(url => !isPathSourceJar(url))
-    binarySource.filter(path => exists(path)).map(_.wrapped.toString)
+    binarySource.filter(path => exists(path)).map(_.toNIO.toUri.toString)


I'm wondering why we don't use a better return type here. At least, we should add some Scaladoc to line 126 to make it clear, that the returned Strings are meant to be URIs.

camper42 · 2021-05-18T07:31:08Z

As it seems this code isn't covered by any tests, I'd like to have at least one written "It works for me" from someone who really tested this locally, for each platform.

will try to add an ut for at least getMillBuildClasspath first, If it seems to take a long time to do, I'm happy to write "It works for me" to get this fix merged.

ScalaWilliam · 2021-05-18T08:00:07Z

My apologies, I did not get a chance to test this.

…path is URIs

camper42 · 2021-05-18T09:31:42Z

hmmm，getMillBuildClasspath on TestEvaluator.static(TestModule).evaluator return empty list
so test always pass....

camper42 · 2021-05-18T10:49:57Z

getMillBuildClasspath on TestEvaluator.static(TestModule).evaluator return empty list

test is running on a version like 0.9.7-4-c85970-DIRTY55902d6c, which can't access before ci/publish-local.sh, so:

classpath in getMillBuildClasspath is empty due to Try(evaluator.rootModule.getClass.getClassLoader.asInstanceOf[SpecialClassLoader]) failed
millJars in getMillBuildClasspath is empty due to Resolution failed

It's impossiable to test, choose to leave a "It works for me on MacOS IDEA" comment...

camper42 · 2021-05-18T12:38:00Z

also worked for my Windows IDEA

test-windows (8, cmd /C %GITHUB_WORKSPACE%\ci\mill.bat -i -d -k "{main,scalajslib,bsp}.__.test") failure can be ignored?
nothing change about mill.scalajslib

lefou

The CI failure is some flakiness in GitHub Actions, I reran the tests, which are now green.

I think the test issue(s) can be solved by propagating the freshly built jars into the test setup via forkEnv and/or forkArgs. We do this in other integration tests and it works. But I think it's too much to ask for in a bug fix PR and your "works for me" is better than what we have currently. We have lots of open building sites in our BSP story, and integration testing is a central part of it.

Your technical change looks good to me, but the Scaladoc needs to be moved before the start of the def on line 126, not inside the def. Otherwise, we're good to merge.

fabianhjr · 2021-05-19T01:50:28Z

Had same issue on Linux (NixOS), ( java.lang.IllegalArgumentException: Missing scheme)

Tested this PR by:

Checking out this repo
Checking out 0.9.7
Cherrypicking this change
Retagging 0.9.7 to include this change
hand modifying a local project .bsp/mill.json to point to a symbolic link to the run script (/home/fabian/.local/bin/mill-checkout -> /home/fabian/Development/Open/mill/out/dev/launcher/dest/run)
Restarting IntelliJ IDEA

This did fix the error and I was able to import the project again; local retagging was to avoid attempting to fetch nonexistant versions of mill-contrib-{bloop, scoverage} (0.9.7-[n]-[local commit that isn't available elsewhere])

camper42 · 2021-05-19T02:49:31Z

@fabianhjr would you mind if I add "works for @fabianhjr on Linux(NixOS)" ?

fabianhjr · 2021-05-19T03:02:54Z

Not at all, go ahead.

Many thanks for the diagnosis and fix of this issue! UwU

[skip ci]

lefou · 2021-05-19T06:35:31Z

Thanks you!

sequencer · 2021-05-24T07:45:29Z

When can we get this patch in a new version like 0.9.8? Since 0.9.7 of mill+bsp+IDEA is totally broken since this issue.

lefou · 2021-05-24T08:13:02Z

You can start to use it right away. Just pick the latest snapshot-build version from https://github.com/com-lihaoyi/mill/releases (e.g. 0.9.7-13-bf96a1) and put it into .mill-version in your project top directory. See https://com-lihaoyi.github.io/mill/mill/Intro_to_Mill.html#_overriding_mill_versions

lefou · 2021-05-24T08:14:08Z

I'd like to get some other fixes into main branch before the next tag.

sequencer · 2021-05-24T08:15:55Z

Got it! Thank you!
I'll use 0.9.7-13-bf96a1 fix this temporarily, since I'm using mill package from Arch Linux.

sequencer · 2021-05-24T08:17:45Z

opps, seems that package from Arch Linux doesn't honor 0.9.7-13-bf96a1

lefou reviewed May 18, 2021

View reviewed changes

camper42 added 2 commits May 18, 2021 17:10

add a scaladoc to describe that strings returned by getMillBuildClass…

cf1802b

…path is URIs

add tests to check strings returned by getMillBuildClasspath

c85970c

remove test & leave a comment instead

1c07727

lefou reviewed May 18, 2021

View reviewed changes

update scaladoc & comment

2a42c72

[skip ci]

lefou merged commit dc1cea1 into com-lihaoyi:main May 19, 2021

lefou added this to the after 0.9.7 milestone May 19, 2021

camper42 deleted the fix-bsp-getMillBuildClasspath branch May 19, 2021 16:40

lefou mentioned this pull request May 25, 2021

Illegal character in URI using BSP #1333

Closed

lefou linked an issue May 25, 2021 that may be closed by this pull request

Illegal character in URI using BSP #1333

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bsp: getMillBuildClasspath return URI string #1325

Fix bsp: getMillBuildClasspath return URI string #1325

camper42 commented May 18, 2021

camper42 commented May 18, 2021

lefou left a comment

lefou May 18, 2021

camper42 commented May 18, 2021

ScalaWilliam commented May 18, 2021

camper42 commented May 18, 2021

camper42 commented May 18, 2021

camper42 commented May 18, 2021

lefou left a comment

fabianhjr commented May 19, 2021 •

edited

Loading

camper42 commented May 19, 2021

fabianhjr commented May 19, 2021

lefou commented May 19, 2021

sequencer commented May 24, 2021

lefou commented May 24, 2021

lefou commented May 24, 2021

sequencer commented May 24, 2021

sequencer commented May 24, 2021

Fix bsp: getMillBuildClasspath return URI string #1325

Fix bsp: getMillBuildClasspath return URI string #1325

Conversation

camper42 commented May 18, 2021

camper42 commented May 18, 2021

lefou left a comment

Choose a reason for hiding this comment

lefou May 18, 2021

Choose a reason for hiding this comment

camper42 commented May 18, 2021

ScalaWilliam commented May 18, 2021

camper42 commented May 18, 2021

camper42 commented May 18, 2021

camper42 commented May 18, 2021

lefou left a comment

Choose a reason for hiding this comment

fabianhjr commented May 19, 2021 • edited Loading

camper42 commented May 19, 2021

fabianhjr commented May 19, 2021

lefou commented May 19, 2021

sequencer commented May 24, 2021

lefou commented May 24, 2021

lefou commented May 24, 2021

sequencer commented May 24, 2021

sequencer commented May 24, 2021

fabianhjr commented May 19, 2021 •

edited

Loading