Skip to content
This repository has been archived by the owner. It is now read-only.

Do not redirect the env command to set, for windows bash shell #19418

Closed
wants to merge 1 commit into from

Conversation

@gucong3000
Copy link
Contributor

@gucong3000 gucong3000 commented Dec 18, 2017

  • Redirect the env only for window cmd shell
  • Add support %ProgramFiles%\Git\usr\bin\bash.exe for lib/utils/is-windows-bash.js

#19419

@gucong3000 gucong3000 requested a review from as a code owner Dec 18, 2017
@KenanY KenanY mentioned this pull request Dec 18, 2017
13 tasks
@gucong3000
Copy link
Contributor Author

@gucong3000 gucong3000 commented Jan 19, 2018

Is there anything I can do to help this PR?

@gucong3000 gucong3000 changed the title using env when has script-shell (Windows) Do not redirect the env command to set, in Cygwin (Windows) Feb 10, 2018
@iarna
Copy link
Contributor

@iarna iarna commented Apr 12, 2018

Hi @gucong3000, thank for your patience! While we don't officially support cygwin, I'm ok with taking patches for it that we can be confident won't cause other issues. In this case, I would suggest you add your Cygwin detection to lib/utils/is-windows-bash.js and then use that in the if statement here.

@@ -138,7 +138,7 @@ function run (pkg, wd, cmd, args, cb) {
if (cmd === 'test') {
pkg.scripts.test = 'echo \'Error: no test specified\''
} else if (cmd === 'env') {
if (process.platform === 'win32') {
if (process.platform === 'win32' && !/^(msys|cygwin)$/.test(process.env.OSTYPE)) {
Copy link
Contributor

@iarna iarna Apr 12, 2018

Use lib/utils/is-windows-bash.js and update that to include cygwin.

@gucong3000 gucong3000 changed the title Do not redirect the env command to set, in Cygwin (Windows) Do not redirect the env command to set, for windows bash shell May 2, 2018
@gucong3000
Copy link
Contributor Author

@gucong3000 gucong3000 commented May 2, 2018

@iarna Thanks for your patience. Modified.

@@ -1,3 +1,4 @@
'use strict'
var isWindows = require('./is-windows.js')
module.exports = isWindows && /^MINGW(32|64)$/.test(process.env.MSYSTEM)
var env = process.env
module.exports = isWindows && (env.MSYSTEM ? /^MINGW(32|64)$/.test(env.MSYSTEM) : env.TERM === "cygwin")
Copy link
Contributor

@iarna iarna Jul 10, 2018

The env.MSYSTEM guard here is unnecessary… but ah, I'll take and make a small change. =)

iarna added a commit to npm/cli that referenced this issue Jul 10, 2018
@iarna
Copy link
Contributor

@iarna iarna commented Jul 10, 2018

Thanks! This is landing as 4c32413!

@iarna iarna closed this Jul 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants