Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

Enhancement: Use win32 platform specific casing for env.path if present #29

Closed
mattezell opened this issue Apr 18, 2019 · 2 comments
Closed

Comments

@mattezell
Copy link
Contributor

There are certain scenarios that may give rise to difficult to diagnose issues stemming from path inconsistency issues introduced further upstream in the npm pipeline before landing in lifecycle.

While it's not lifecycle's responsibility to sanitize env pairs, it would be nice if lifecycle preferred OS specific path casing when it's present, vs iterating through all case insensitive matches before using on the last match.

Consider the following being set upstream from lifecycle:

process.env['Path'] = 'c:\\path\\one';
process.env['PATH'] = 'c:\\path\\two';

When instantiated, lifecycle currently evaluates process.env keys with a case insensitive regex match within a forEach - selecting the last match as the path casing that will be used for the remainder of lifecycle processes (including modifying the path with module specific paths). In this scenario, 'PATH' will be used within lifecycle - though ultimately process.env will be passed containing both 'PATH' and 'Path' - at which time win32 platforms will use 'Path' when instantiating the native a Process() instance. Of course, since lifecycle opted for 'PATH' due to it being the last case insensitive match, path modifications that may be required won't be picked up by Process(), which may lead to failure.

I have personally dealt with such a failure, which ultimately resulted in a difficult to diagnose issue with the increasingly popular opencollective and lightcollective libraries being called in postinstall by 3rd party dependencies. While this issue may be mitigated by authors simply adding "|| exit 0" to their postinstall calls to external libraries, not all authors are easily convinced to make this change. There are many reported issues surrounding opencollective and lightercollective erroring with "is not recognized as an internal or external command", specifically on Windows - I figure that it's likely that this is frequently an underlying cause.

I propose that lifecycle use the OS-specific path casing when present on process.env - only falling back to iterating through the keys in search of an alternate when the OS-specific casing isn't present. Pull request imminent.

mattezell added a commit to mattezell/document-register-element that referenced this issue Apr 19, 2019
Various environmental or project scenarios may lead to a failure when calling lightercollective from postinstall. This is not known to be a lightercollective failure, but instead of a failure to properly call the lightercollective library. One such known scenario on Windows being multiple path casings being passed to child_process.spawn when npm-lifecycle processes postinstall.

Discussion on lifecycle/postinstall Win32 issue here: npm/npm-lifecycle#29

This PR is intended to address this discussion: WebReflection#165
@mattezell
Copy link
Contributor Author

mattezell commented Apr 22, 2019

@zkat or @aeschright is this the appropriate place for PRs for npm-lifecycle? After submitting this last week, I notice this repo doesn't appear to get much activity, which led me to question if I'd perhaps submitted this to the wrong place. Thanks!

@mattezell
Copy link
Contributor Author

@isaacs, do you know these repos are actively monitored/maintained? Is there a more appropriate place to open issues and submit PRs? Thanks!

@isaacs isaacs closed this as completed in 50462c3 Jul 17, 2019
isaacs pushed a commit that referenced this issue Jul 17, 2019
Assume proper win32 path casing - only seek out alternative casing if
win32 OS specific casing not present.

Fix: #29
PR-URL: #30
Close: #30
Reviewed-by: @isaacs
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant