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 InvalidPathException using BSP on Windows. #1092

Closed
wants to merge 3 commits into from
Closed

Fix InvalidPathException using BSP on Windows. #1092

wants to merge 3 commits into from

Conversation

Iltotore
Copy link
Contributor

@Iltotore Iltotore commented Jan 3, 2021

Iltotore and others added 3 commits December 4, 2020 15:57
Upgrade bsp4j and implement buildTarget/javacOptions and fix build/ta…
Pull changes from parent repository
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Looks reasonable. I'd would be great, to also have a test case for that.

@Iltotore
Copy link
Contributor Author

Iltotore commented Jan 4, 2021

Looks reasonable. I'd would be great, to also have a test case for that.

Yes. Paths can be capricious between operating systems. Maybe this test and change should be applied to the oslib.

@lefou
Copy link
Member

lefou commented Jan 7, 2021

Looks reasonable. I'd would be great, to also have a test case for that.

Yes. Paths can be capricious between operating systems. Maybe this test and change should be applied to the oslib.

@Iltotore Are you going to create a PR for oslib?

@Iltotore
Copy link
Contributor Author

Iltotore commented Jan 7, 2021

Looks reasonable. I'd would be great, to also have a test case for that.

Yes. Paths can be capricious between operating systems. Maybe this test and change should be applied to the oslib.

@Iltotore Are you going to create a PR for oslib?

If you think it is better than pushing changes here, yes.

@lefou
Copy link
Member

lefou commented Jan 7, 2021

It's a misbehavior there and should be fixed there.

@lefou lefou added the upstream The issue originates in an upstream dependency label Jan 20, 2021
@lefou lefou marked this pull request as draft January 20, 2021 15:59
ScalaWilliam added a commit to ScalaWilliam/mill that referenced this pull request Apr 18, 2021
…buildTarget/dependencySources - cannot import into IDE

There is a PR already (com-lihaoyi#1092) but I am wary of dealing with Strings directly. Here is my approach.

On Windows, File URLs follow an peculiar representation in Java

```
scala> java.nio.file.Paths.get(".").toAbsolutePath.toUri.toURL
java.net.URL = file:/C:/Users/Developer/mill/./
```

From this, we wish to get a `Path` back, and the way to do this is:

```
scala> java.nio.file.Paths.get((java.nio.file.Paths.get(".").toAbsolutePath.toUri.toURL).toURI)
java.nio.file.Path = C:\Users\Developer\mill\.
```

Unit testing for this is more challenging because the `WindowsFileSystem` instance is a `sun.nio.fs` package, rather than a standard package.

The solution here, compared to the previous code, is to reduce the number of conversions; the key loss happens when you do `(URL).getFile`.

```
scala> java.nio.file.Paths.get(".").toAbsolutePath.toUri.toURL
java.net.URL = file:/C:/Users/Developer/mill/./
scala> java.nio.file.Paths.get(".").toAbsolutePath.toUri.toURL.getFile
String = /C:/Users/Developer/mill/./
```
lefou pushed a commit that referenced this pull request Apr 20, 2021
…Sources - cannot import into IDE (#1285)

There is a PR already (#1092) but I am wary of dealing with Strings directly. Here is my approach.

On Windows, File URLs follow an peculiar representation in Java

```
scala> java.nio.file.Paths.get(".").toAbsolutePath.toUri.toURL
java.net.URL = file:/C:/Users/Developer/mill/./
```

From this, we wish to get a `Path` back, and the way to do this is:

```
scala> java.nio.file.Paths.get((java.nio.file.Paths.get(".").toAbsolutePath.toUri.toURL).toURI)
java.nio.file.Path = C:\Users\Developer\mill\.
```

Unit testing for this is more challenging because the `WindowsFileSystem` instance is a `sun.nio.fs` package, rather than a standard package.

The solution here, compared to the previous code, is to reduce the number of conversions; the key loss happens when you do `(URL).getFile`.

```
scala> java.nio.file.Paths.get(".").toAbsolutePath.toUri.toURL
java.net.URL = file:/C:/Users/Developer/mill/./
scala> java.nio.file.Paths.get(".").toAbsolutePath.toUri.toURL.getFile
String = /C:/Users/Developer/mill/./
```

Pull request: #1285
@lefou
Copy link
Member

lefou commented Apr 20, 2021

Superseded by #1285 and also by upstream PR com-lihaoyi/os-lib#53.

@lefou lefou closed this Apr 20, 2021
@lefou
Copy link
Member

lefou commented Apr 21, 2021

@Iltotore Thank you for your work on this topic! Although we fixed the issue now without any of your commits, your work is highly appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream The issue originates in an upstream dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants