dmd.root.filename: Fix portability issues with FileName.canonicalName. #9261
Conversation
|
Thanks for your pull request, @ibuclaw! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + dmd#9261" |
For Posix platforms, canonicalName relies on that most libc implementations return an internally malloc'd buffer for us when passing realpath a null destination pointer. However, the specification does not guarantee this, and broken behaviour is observed on some platforms, such as Solaris. Always providing a buffer is the only way to ensure that calling realpath is consistent across platforms.
| // unless there is nothing to copy from. | ||
| if (!name.length) | ||
| return null; | ||
| return mem.xstrdup(name.ptr)[0 .. name.length]; |
There was a problem hiding this comment.
Why not a static assert instead of doing nothing ? It does not look like a good default behavior and might be hard to track.
There was a problem hiding this comment.
Just returning null would mean nothing would compile. We already have the relative paths to druntime and phobos, let the compiler use them instead of erroring that it can't find any sources.
| import core.sys.posix.stdlib: free; | ||
| auto absPath = FileName.canonicalName(fileName); | ||
| scope(exit) free(cast(void*)absPath.ptr); | ||
| return F(cast(char[])absPath); |
There was a problem hiding this comment.
It would be simpler just to swap lines 1130 and 1131:
F(..)
free(...)
There was a problem hiding this comment.
If this was a void function, then I'd agree.
|
@ibuclaw I'm not sure, but this PR may be the cause of this regression: https://issues.dlang.org/show_bug.cgi?id=19684. Summary: In 2.085.0-beta.1 DMD can no longer find |
|
@jondegenhardt - could you bisect or confirm with a |
|
@ibuclaw - It'll be a day before I can try to do any debugging. Sorry I can't be more definitive about whether this PR is the cause. It seemed the mostly likely of all the PRs that went into the 2.085 release. |
|
OK. If there's any problem, it may be to do with the version of dmd used to build 2.085. The static paths work just fine, but the conditions may evaluate to false if druntime is too old. The release binaries for 2.085 should be bootstrapped from 2.085 itself, otherwise I'd argue that dmd maintainers are doing something wrong. |
|
On OSX, druntime 2.085 defines PATH_MAX (prior versions didn't). There is no definition for _PC_PATH_MAX for OSX in druntime at all (this would be a bug in the OSX port). |
Currently the latest stable release (e.g. 2.084.1) is used to build DMD and as we will hopefully eventually build the shipped binaries with LDC this assumption won't hold in this future either. |
Building with LDC won't fix the problem Jon found if I understood what's going on correctly. |
No, building with LDC would be to get better performance. I'm pretty sure that if/when LDC is used they will have caught up with the 2.085.0 fronted version. |
8 participants
For Posix platforms, canonicalName relies on that most libc implementations return an internally malloc'd buffer for us when passing realpath a null destination pointer.
However, the specification does not guarantee this, and broken behaviour is observed on some platforms, such as Solaris.
Always providing a buffer is the only way to ensure that calling realpath is consistent across platforms.
See dlang/druntime#2460 for PR that adds
PATH_MAXto druntime.Each static path has been tested for correctness on Linux 64bit using glibc.