Fallback to process cwd when resolving drive cwd #8541
Conversation
|
Are you able to add a test for this? |
The `path.resolve()` function when given just a drive letter such as "C:" tries to get a drive-specific CWD, but that isn't available in cases when the process is not launched via cmd.exe and the process CWD has not been explicitly set on that drive. This change adds a fallback to the process CWD, if the process CWD happens to be on the resolved drive letter. If the process CWD is on another drive, then a drive-specific CWD cannot be resolved and defaults to the drive's root as before. Based on experimentation, the fixed behavior matches that of other similar path resolution implementations on Windows I checked: .NET's `System.IO.Path.GetFullPath()` and Python's `os.path.abspath()`. In the automated path test cases the issue doesn't occur when the tests are run normally from cmd.exe. But it did cause an assertion when running the tests from PowerShell, that is fixed by this change. Fixes: nodejs#7215
jasongin
force-pushed
the
resolvedrive
branch
from
66104dd to
6413e1e
Compare
|
@Trott OK, I added a test. |
|
@nodejs/platform-windows (helps to look at #7215 for context of the problem too) |
|
Are there probable semver implications to this? Might a Windows CIGTM run be in order? Or would that be a bit much? |
|
@Trott Is citgm on windows currently working? Judging from the current status of nodejs/citgm#157 I'd assume not. |
|
@gibfahn Ugh, yes, you appear to be correct. I don't see any Windows hosts at https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/. Oh well. Anyway, are there likely semver implications here? Seems like "probably not" to me, but I'm not exacxtly Windows-savvy. |
|
The previous behavior was objectively wrong/buggy, because it would (sometimes!) resolve "C:" to "C:" when the current working directory was "C:\path...". It is remotely possible that some user code relied on that buggy behavior. However I think that's extremely unlikely because the bug was not even consistent: it only occurred in a spawned process and only if the process cwd had not been explicitly set. So in my opinion this should probably not be considered a breaking change. |
|
That works for me! LGTM |
|
This might be a bit of a gray area because behavior changes, but I completely agree with @jasongin that this is a bug fix. I cannot think of any situation where good code would break because of this. |
|
Landing:
|
The `path.resolve()` function when given just a drive letter such as "C:" tries to get a drive-specific CWD, but that isn't available in cases when the process is not launched via cmd.exe and the process CWD has not been explicitly set on that drive. This change adds a fallback to the process CWD, if the process CWD happens to be on the resolved drive letter. If the process CWD is on another drive, then a drive-specific CWD cannot be resolved and defaults to the drive's root as before. Based on experimentation, the fixed behavior matches that of other similar path resolution implementations on Windows I checked: .NET's `System.IO.Path.GetFullPath()` and Python's `os.path.abspath()`. In the automated path test cases the issue doesn't occur when the tests are run normally from cmd.exe. But it did cause an assertion when running the tests from PowerShell, that is fixed by this change. PR-URL: nodejs#8541 Fixes: nodejs#7215 Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: James M Snell <[email protected]>
The `path.resolve()` function when given just a drive letter such as "C:" tries to get a drive-specific CWD, but that isn't available in cases when the process is not launched via cmd.exe and the process CWD has not been explicitly set on that drive. This change adds a fallback to the process CWD, if the process CWD happens to be on the resolved drive letter. If the process CWD is on another drive, then a drive-specific CWD cannot be resolved and defaults to the drive's root as before. Based on experimentation, the fixed behavior matches that of other similar path resolution implementations on Windows I checked: .NET's `System.IO.Path.GetFullPath()` and Python's `os.path.abspath()`. In the automated path test cases the issue doesn't occur when the tests are run normally from cmd.exe. But it did cause an assertion when running the tests from PowerShell, that is fixed by this change. PR-URL: #8541 Fixes: #7215 Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
I'm going to make the assumption that v4.x is not affected by this as this patch is not landing cleanly. If I am mistaken please feel free to backport |
|
@thealphanerd I believe it is affected as well. That code hasn't changed and it not related to any of the realpath stuff since we are now back on the JS implementation. |
The `path.resolve()` function when given just a drive letter such as "C:" tries to get a drive-specific CWD, but that isn't available in cases when the process is not launched via cmd.exe and the process CWD has not been explicitly set on that drive. This change adds a fallback to the process CWD, if the process CWD happens to be on the resolved drive letter. If the process CWD is on another drive, then a drive-specific CWD cannot be resolved and defaults to the drive's root as before. Based on experimentation, the fixed behavior matches that of other similar path resolution implementations on Windows I checked: .NET's `System.IO.Path.GetFullPath()` and Python's `os.path.abspath()`. In the automated path test cases the issue doesn't occur when the tests are run normally from cmd.exe. But it did cause an assertion when running the tests from PowerShell, that is fixed by this change. PR-URL: #8541 Fixes: #7215 Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: James M Snell <[email protected]>
Checklist
vcbuild test nosign(Windows) passes (Some tests failed, but they're unrelated: the failures repro without this change.)Affected core subsystem(s)
path
Description of change
The
path.resolve()function when given just a drive letter such as "C:" tries to get a drive-specific CWD, but that isn't available in cases when the process is not launched via cmd.exe and the process CWD has not been explicitly set on that drive.This change adds a fallback to the process CWD, if the process CWD happens to be on the resolved drive letter. If the process CWD is on another drive, then a drive-specific CWD cannot be resolved and defaults to the drive's root as before.
Based on experimentation, the fixed behavior matches that of other similar path resolution implementations on Windows I checked: .NET's System.IO.Path.GetFullPath() and Python's os.path.abspath().
Fixes: #7215