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 #5129: Use system-dependent path separator #5130

Merged
merged 1 commit into from Oct 2, 2018

Conversation

Projects
None yet
6 participants
@melekhove
Copy link
Contributor

commented Sep 20, 2018

No description provided.

@dotty-bot
Copy link

left a comment

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

@@ -12,10 +12,10 @@ final case class TestFlags(
TestFlags(defaultClassPath, runClassPath, options diff flags)

def withClasspath(classPath: String): TestFlags =
TestFlags(s"$defaultClassPath:$classPath", runClassPath, options)
TestFlags(s"$defaultClassPath${sys.props("path.separator")}$classPath", runClassPath, options)

This comment has been minimized.

Copy link
@allanrenucci

allanrenucci Sep 20, 2018

Member

Isn't java.io.File.pathSeparator working?

@melekhove melekhove force-pushed the melekhove:issue/5129 branch 2 times, most recently from cf96f61 to a8cd069 Sep 20, 2018

@@ -54,7 +54,7 @@ trait ParallelTesting extends RunnerOrchestration { self =>
def outDir: JFile
def flags: TestFlags

def runClassPath: String = outDir.getAbsolutePath + ":" + flags.runClassPath
def runClassPath: String = outDir.getAbsolutePath + java.io.File.pathSeparator + flags.runClassPath

This comment has been minimized.

Copy link
@Blaisorblade

Blaisorblade Sep 21, 2018

Contributor

File is already imported as JFile, please use that.


def withRunClasspath(classPath: String): TestFlags =
TestFlags(defaultClassPath, s"$runClassPath:$classPath", options)
TestFlags(defaultClassPath, s"$runClassPath${java.io.File.pathSeparator}$classPath", options)

This comment has been minimized.

Copy link
@Blaisorblade

Blaisorblade Sep 21, 2018

Contributor

I’d prefer if you import File here too (maybe as JFile for consistency).

@Blaisorblade

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

And the commit message should be updated (you’re not using a system property); and please say “fix #5129” rather than “fix issue #5129”.

@Blaisorblade
Copy link
Contributor

left a comment

Requesting changes detailed above.

@melekhove melekhove force-pushed the melekhove:issue/5129 branch from a8cd069 to 101acc6 Sep 22, 2018

@melekhove melekhove changed the title Fix issue #5129: Use system property for classpath separator Fix #5129: Use system-dependent path-separator Sep 22, 2018

@melekhove melekhove force-pushed the melekhove:issue/5129 branch from 101acc6 to f057fe2 Sep 24, 2018

@melekhove melekhove changed the title Fix #5129: Use system-dependent path-separator Fix #5129: Use system-dependent path separator Sep 24, 2018

@melekhove melekhove force-pushed the melekhove:issue/5129 branch from f057fe2 to 87b5c42 Sep 24, 2018

@sjrd

sjrd approved these changes Sep 24, 2018

Copy link
Member

left a comment

Thanks a lot!

It's not completely enough for testCompilation to run successfully on my machine (log) but at least now it does most of it right (as opposed to everything wrong before).

@melekhove

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

BTW java.nio.file.NoSuchFileException: ...i4947c\Aux.tasty in the log is the result of the #5148

@melekhove

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

See also #5155 and #5157

@melekhove melekhove force-pushed the melekhove:issue/5129 branch 2 times, most recently from c5f7c80 to b89343c Sep 25, 2018

@melekhove melekhove force-pushed the melekhove:issue/5129 branch from b89343c to f7946bb Oct 1, 2018

@melekhove

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2018

And the commit message should be updated (you’re not using a system property); and please say “fix #5129” rather than “fix issue #5129”.
I've made all required changes

@OlivierBlanvillain

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2018

@melekhove Merging, thanks!

@OlivierBlanvillain OlivierBlanvillain merged commit 8cc445b into lampepfl:master Oct 2, 2018

2 checks passed

CLA User signed CLA
Details
continuous-integration/drone/pr the build was successful
Details

@melekhove melekhove deleted the melekhove:issue/5129 branch Oct 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.