fixed #252

[PatrickBuTaxdoo]

See #252

Fixed a bug when loading requirements.txt on windows machines with dockerizePip set to false

[dschep]

@heri16 @kichik, you're kinda my resident Windows experts. Does this change make sense to you?

[AndrewFarley]

Not an expert on Windows, but I'd say this looks good, it should always need to be escaped I believe. In similar news @dschep any ideas on bootstrapping a CI script / env on Windows? I did manage to setup a docker toolbox on my windows box here, so I can do tests before releases, we just need to script the existing tests in a windows-friendly way. I can't seem to find a web based CI or cloud provider that has windows with docker, maybe there is one though. Seems like a gap in the CI market, but I'd like to make our Windows guys stop feeling abused from breaking builds. :P

[dschep]

re CI - yes. I have a WIP of porting the tests from bats to tape which should make the tests runnable on windows. Docker support will be trickier, but even just non-docker tests will be WAAYYY more covg than we have now 😄

[kichik]

I think this would hide the actual error. Instead of \\, we would have // which Windows seems to handle better in this case. But if the path to convert would be a network path (starting with double back slashes - \\) this code might change the meaning of the path.

@PatrickBuTaxdoo do you know why the path ends up with double slashes in the first place? Is the single_quote() function messing it up or was it like that before?

On a separate note, dockerPathForWin() might need a new name if it's used for both Docker and non-Docker paths.

[PatrickBuTaxdoo]

@kichik I think the single_quote function is messing up.

I modified the function like this:

function dockerPathForWin(options, path) {
  console.log(path);
  console.log(quote_single(path));
  console.log(path.replace(/\\/g, '/'));
  if (process.platform === 'win32') {
    return path.replace(/\\/g, '/');
  }
  return quote_single(path);
}

And got this additional output:

C:\Users\[...]\.serverless\requirements
C\:\\Users\\[...]\\.serverless\\requirements
C:/Users/[...]/.serverless/requirements
C:\Users\[...]\.serverless\requirements\requirements.txt
C\:\\Users\\[...]\\.serverless\\requirements\\requirements.txt
C:/Users/[...]/.serverless/requirements/requirements.txt

So the path before passing it to single_quote was just fine.

But if figured that it works without modifying the path for my configuration, so this fix should be more appropriate as it is less likely to break compatibility with docker builds:

function dockerPathForWin(options, path) {
  if (process.platform === 'win32' && options.dockerizePip) {
    return path.replace(/\\/g, '/');
  } else if (process.platform === 'win32' && !options.dockerizePip) {
    return path
  }
  return quote_single(path);
}

I added a commit to this PR.

[kichik]

So the other thing quote_single() should be doing is quoting paths with spaces. I think it might be better to find a library that handles this quoting properly on both Windows and Linux. Otherwise we'd have to handle escaping unique characters and spaces ourselves.

[dschep]

So, my initial searching doesn't seem to indicate thare are any shell escape libraries for windows, @kichik. 😦

[dschep]

So, I'm gonna merge this for the time being. Better than nothing.