-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issue 2508 #2510
Fix issue 2508 #2510
Conversation
Conflicts: bin/lessc
id of maybe gone for replace(/^'|'$/g, '') and not assigned to another array, but this is okay, thanks. |
Ah, there are jshint warnings bin/lessc You need to var the filenames or it will be global |
sorry bout that. fixed.
Just for the future references: This PR does not completely "fix" it actually. It still fails with The problem is that for such input the args passed in are:
and the new code does nothing in this case. I guess the only way to fix it as intended is to parse the raw string of all arguments from scratch as a whole. P.S. After a bit of thinking it seems that the proper way to fix all this would be to hardly narrow it to: ^ Looks like a nice price for someone's dainty Python script coding :) |
This PR has a few more issues than what @seven-phases-max is stating: It introduces a problem where the compiler crashes with an exception when you don't specify an output file. I tracked it down to a missing null-reference check on Also, quoted for emphasis:
That means you cannot safely remove it as a character without affecting potentially valid filenames. Furthermore, outside of one occurence (namely; Infact; the Windows commandline doesn't even support multiple arguments. It only supports one argument and any splitting into multiple arguments is sugar added by libraries. There are some system tools that, as part of this sugar, allow usage of single quotes to be used interchangeably but it is not standard. The standard convention is to follow what the Microsoft C library and the CommandLineToArgvW function do and that lists ONLY double-quotes as having special behavior. Me thinks whatever broken library @wahuneke is using in #2508 to call out to Lessc, should receive a pull-request to fix their busted behavior instead. |
Yep, and that's exactly why we recieve |
@rjgotten - The problem is that the fault boils down to code in Python 2.7 (for windows). This is rather widely used and not easily patched. The code is in pipes.py, in quote(). This code is in turn called by django-pipeline, a very popular library for Django developers (such as myself). Earlier, here and in the bug ticket I made the argument that python2.7 and quote() are widely used, will be used with Less, that this problem will probably bug a bunch of people and that nobody's going to be using a single quote in their filenames, especially at the end of their filename (e.g. o'reilly's less file.less is possible, but coolStyles.less' would be a pretty weird filename). My argument was based on expediency and I estimated that this was just a 'judement call'. Looks like maybe I was wrong. @seven-phases-max - I think you made this argument before, but I only just now follow you. I guess your point boils down to: if someone has a space in their filename and Python goes and puts single quotes around it, MS is going to interpret the filename differently when it passes it in to node.exe and less will not be able to find the file. Right-o... hmmmm..... That inspires me to look at the docs for Python 2: https://docs.python.org/2/library/pipes.html Well hey look at that! quote() is only intended for use in /bin/sh style command lines. The fault therefore lies with django-pipeline (and with me for bringing this up). django-pipeline (and any python lib) should not use quote() for cross platform libs as far as I can tell. Sorry everybody. Thanks for your patience. I hereby add my vote to those suggesting that this PR be reverted. |
Sounds like a good idea. EDIT: oop, I see it was already reverted. |
Also, btw, if the PR passed existing tests, we should add tests for the cases where this failed, no? |
We currently have no tests for the lessc entry point.
|
This new pull request is much cleaner. Not a bunch of "oops" commit messages :)
This pull request fixes #2508