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

compilers using pipes.quote() == not Windows compatible. breaks LESSc #444

Closed
wahuneke opened this Issue Mar 20, 2015 · 2 comments

Comments

Projects
None yet
2 participants
@wahuneke

wahuneke commented Mar 20, 2015

In bug #335, a call to quote() was added to better handle variety of filenames. It turns out that quote() is not windows compatible and is not intended to be (neither the shlex version nor the pipes version).

quote() adds single quote characters, windows requires double quotes as single quotes have no special meaning (to windows it's just an apostrophe, and apostrophes are ok in windows filenames).

For me, pipeline is adding single quotes everywhere it calls lessc. This is breaking lessc, which does not handle the single quotes properly. It tries to add the current working directory to the quoted filename and comes up with a brand new filename that is not valid.

To me, things would be much simpler if you called lessc using Popen in 'not shell' mode. Pass in the name of the executable, pass in the arguments as an array. Dont try to quote anything. dont turn the whole thing into one long command string. Is there a reason it was not implemented this way?

I have already been working with lessc. They merged a patch for me which "fixes" the single quote issue. But now we are removing it because it breaks to deviate from the windows specs in this way.

I can submit a PR, just wanted to get feedback first if possible.

wonder if this is related to issue #368 ...

@dwatkinsweb

This comment has been minimized.

Show comment
Hide comment
@dwatkinsweb

dwatkinsweb Apr 15, 2015

I am getting this same issue and was just about to try and figure out a resolution myself. If you have one I recommend setting a pull request and I can help test as well. Otherwise, I can assist in resolving this issue if needed. My biggest worry, I'm not sure the best way to keep this backwards compatible.

dwatkinsweb commented Apr 15, 2015

I am getting this same issue and was just about to try and figure out a resolution myself. If you have one I recommend setting a pull request and I can help test as well. Otherwise, I can assist in resolving this issue if needed. My biggest worry, I'm not sure the best way to keep this backwards compatible.

dwatkinsweb pushed a commit to dwatkinsweb/django-pipeline that referenced this issue Apr 15, 2015

David Watkins
jazzbandGH-444 - using pipes.quote() breaks windows compatibility
Convert command to use a list of arguments and sending outfile to stdout
for compilers using it.

dwatkinsweb pushed a commit to dwatkinsweb/django-pipeline that referenced this issue Apr 15, 2015

David Watkins
jazzbandGH-444 - using pipes.quote() breaks windows compatibility
Convert command to use a list of arguments and sending outfile to stdout
for compilers using it.
@wahuneke

This comment has been minimized.

Show comment
Hide comment
@wahuneke

wahuneke Apr 15, 2015

Yeah, I played around with solutions a while back. Wound up rewriting
things so that lessc, et al no longer would execute in shell mode.
Eventually figured out that this is not ideal because those guys actually
tend to be shell scripts and everything's just a heck of a lot easier if we
execute in shell mode.

Backing that change out, I still did have a solution that removed the
quoting. I will get that pushed as a PR, but probably can't get to that
today.

On Wed, Apr 15, 2015 at 12:53 AM, dwatkinsweb notifications@github.com
wrote:

I am getting this same issue and was just about to try and figure out a
resolution myself. If you have one I recommend setting a pull request and I
can help test as well. Otherwise, I can assist in resolving this issue if
needed.


Reply to this email directly or view it on GitHub
#444 (comment)
.

wahuneke commented Apr 15, 2015

Yeah, I played around with solutions a while back. Wound up rewriting
things so that lessc, et al no longer would execute in shell mode.
Eventually figured out that this is not ideal because those guys actually
tend to be shell scripts and everything's just a heck of a lot easier if we
execute in shell mode.

Backing that change out, I still did have a solution that removed the
quoting. I will get that pushed as a PR, but probably can't get to that
today.

On Wed, Apr 15, 2015 at 12:53 AM, dwatkinsweb notifications@github.com
wrote:

I am getting this same issue and was just about to try and figure out a
resolution myself. If you have one I recommend setting a pull request and I
can help test as well. Otherwise, I can assist in resolving this issue if
needed.


Reply to this email directly or view it on GitHub
#444 (comment)
.

cyberdelia pushed a commit that referenced this issue Dec 29, 2015

Chad Miller Timothée Peignier
Make compilers run directly, not inside shells
Names on disk and config files should not have to be safe for shells to
interpret. As close to possible, we should take literals and give literals to
the OS kernel to operate on directly. For filename globs, it is our
responsiblility to expand them, and if we had no problem with backwards
compatibility, we would insist config files' SCRIPT_ARGUMENTS parameters are
tuples of discrete values. Delegating those to a shell breaks clear boundaries
of interpreetation and will always be prone to errors, oversight, and
incompatibility.

So, now, we never take names that are unsafe and try to make then safe for a
shell, because we don't need a shell.

This fixes #444, which had problems with Windows paths being insensible to the
crazy quoting we hoped would make a filename safe for a shell.

This fixes #494, which had a compiler-attempt stdout captured as part of a
string interpreted by a shell. When the compiler didn't exist, that shell
expression STILL created empty files, and the pipeline thenafter served an
empty file as if it were real compiler output.

cyberdelia pushed a commit that referenced this issue Dec 30, 2015

Chad Miller Timothée Peignier
Make compilers run directly, not inside shells
Names on disk and config files should not have to be safe for shells to
interpret. As close to possible, we should take literals and give literals to
the OS kernel to operate on directly. For filename globs, it is our
responsiblility to expand them, and if we had no problem with backwards
compatibility, we would insist config files' SCRIPT_ARGUMENTS parameters are
tuples of discrete values. Delegating those to a shell breaks clear boundaries
of interpreetation and will always be prone to errors, oversight, and
incompatibility.

So, now, we never take names that are unsafe and try to make then safe for a
shell, because we don't need a shell.

This fixes #444, which had problems with Windows paths being insensible to the
crazy quoting we hoped would make a filename safe for a shell.

This fixes #494, which had a compiler-attempt stdout captured as part of a
string interpreted by a shell. When the compiler didn't exist, that shell
expression STILL created empty files, and the pipeline thenafter served an
empty file as if it were real compiler output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment