broken windows modules path handling because of 'os.relativePath' breaking changes

[jangko]

'os.relativePath' got additional code:

  when doslikeFileSystem:
    if isAbsolute(path) and isAbsolute(base):
      if not sameRoot(path, base):
        return path

this changes also change it's behavior:

import os

let a = r"C:\Users\Jangko\.nimble\pkgs\eth-1.0.0\eth\common\utils.nim"
let b = r"F:\projects\bug"

debugEcho relativePath(a, b, '/')

• Nim 1.0.2 produce: "../../../C:/Users/Jangko/.nimble/pkgs/eth-1.0.0/eth/common/utils.nim"
• Nim 1.1.1 produce: "C:\Users\Jangko.nimble\pkgs\eth-1.0.0\eth\common\utils.nim"

Nim compiler directly affected by this braking changes:

• in pathutils.nim

  proc relativeTo*(fullPath: AbsoluteFile, baseFilename: AbsoluteDir;
                   sep = DirSep): RelativeFile =
    RelativeFile(relativePath(fullPath.string, baseFilename.string, sep))

• in packagehandling.nim

proc fakePackageName*(conf: ConfigRef; path: AbsoluteFile): string =
  # foo-#head/../bar becomes @foo-@hhead@s..@sbar
  result = "@m" & relativeTo(path, conf.projectPath, '/').string.multiReplace({"/": "@s", "#": "@h", "@": "@@", ":": "@c"})

this bug actually detected before but with inconclusive ending.

• [regression] Importing more than one module with same name from different packages produce bad codegen #12420
• test failure with Nim-1.0.4 release candidate status-im/nimbus-eth1#424

how to reproduce:

• prepare a working windows machine + mingw gcc.
• compile nim compiler binary from csources.
• bootstrap a new compiler from that binary.
• do not use previous version compiler with 'good' os.relativePath to boostrap a new compiler.

$> nimble install eth
$> nimble install nimcrypto

then compile this file with nim c --threads:on -d:metrics bug

# bug.nim
import eth/common/utils as autils, nimcrypto/utils

var x: array[100, byte]
burnMem(x)

Current Output

C:\Users\Jangko\nimcache\bug_d\utils.nim.c.o:utils.nim.c:(.text+0x41e): multiple definition of `eth_utilsInit000'
C:\Users\Jangko\nimcache\bug_d\utils.nim.c.o:utils.nim.c:(.text+0x41e): first defined here
C:\Users\Jangko\nimcache\bug_d\@mbug.nim.c.o:@mbug.nim.c:(.text+0x225): undefined reference to `burnMem__haAOjkJjOuRy8ux9bV57dXw'
collect2.exe: error: ld returned 1 exit status

Possible Solution

• os.relativePath need to be fixed
• or compiler internal avoid using the new 'broken' feature.

Additional Information

• It was working in version 1.0.2
• This issue is valid only on Windows.

$ nim -v
Nim Compiler Version 1.1.1 [Windows: amd64]
Compiled at 2019-11-25
Copyright (c) 2006-2019 by Andreas Rumpf

git hash: 015bec625461826c3891110182b982c5dbb3208a
active boot switches: -d:release

[Araq]

We'll revert this "bugfix", I was sceptical about it but tests were green. :P

[demotomohiro]

Here is PR add that code.
#12329

Unlike Unix, Windows is multiple root file system. You cannot create a relative path from 2 absolute paths with different drive.
Following command show error on console in Windows.

d:
cd d:\
dir ..\c:\Windows

From a comment in os.relativePath:

On windows, if a root of path and a root of base are different,
returns path as is because it is impossible to make a relative path.
That means an absolute path can be returned.

When you use os.relativePath, you need to write your code so that it works even if os.relativePath returns absolute path.
You need to check if returned path is absolute before appending it to a directory path.
Should os module provide a proc that take 2 paths and return true only if os.relativePath can make a relative path from given 2 path?

[jangko]

if we modify the last line of the additional code:

 when doslikeFileSystem:
    if isAbsolute(path) and isAbsolute(base):
      if not sameRoot(path, base):
        return path

to

       return path.replace(DirSep, sep)

this seems accepted by compiler internal, probably no need to revert the problematic commits.
and more importantly, no need to add a new proc than can increase compiler internal complexity.
but of course, always measure before make any cuts.

[demotomohiro]

os.relativePath is a proc to convert given path to a relative path.
If it always do both convert to relative path and replace path separater, it seems wrong.
I think third parameter sep of os.relativePath should be used only in output path creation.

I think changing following code in compiler/packagehandling.nim

proc fakePackageName*(conf: ConfigRef; path: AbsoluteFile): string =
  # foo-#head/../bar becomes @foo-@hhead@s..@sbar
  result = "@m" & relativeTo(path, conf.projectPath, '/').string.multiReplace({"/": "@s", "#": "@h", "@": "@@", ":": "@c"})

to

proc fakePackageName*(conf: ConfigRef; path: AbsoluteFile): string =
  # foo-#head/../bar becomes @foo-@hhead@s..@sbar
  result = "@m" & relativeTo(path, conf.projectPath).string.multiReplace({os.DirSep: "@s", os.AltSep: "@s", "#": "@h", "@": "@@", ":": "@c"})

and

proc demanglePackageName*(path: string): string =
  result = path.multiReplace({"@@": "@", "@h": "#", "@s": "/", "@m": "", "@c": ":"})

to

proc demanglePackageName*(path: string): string =
  result = path.multiReplace({"@@": "@", "@h": "#", "@s": os.DirSep, "@m": "", "@c": ":"})

will fix the problem.
But I need more research.

[demotomohiro]

This is minimal code to reproduce same error:

d:\temp\foo\abc.nim

var a* = "foo"

d:\temp\bar\abc.nim

var b* = "bar"

f:\temp\test.nim

import foo/abc
import bar/abc as abc2

echo a, b

Build test.nim with following command:

f:
cd f:\temp
nim c --path:d:\temp -r test.nim

At least, foo\abc.nim and bar\abc.nim need to be placed in a drive different from test.nim.