j0hnsmith / django-pipeline-browserify

MIT License
37 stars 18 forks source link

Invalid compiler command is dispatched in the is_outdated #14

Closed MrCsabaToth closed 7 years ago

MrCsabaToth commented 8 years ago

This https://github.com/j0hnsmith/django-pipeline-browserify/blob/master/pipeline_browserify/compiler.py#L55 yields a single string u' c:\\Users\\JohnSmith\\node_modules\\.bin\\browserify.cmd -t babelify --deps C:\\Users\\JohnSmith\\Documents\\test\\company\\static\\dashboard\\js\\react_test_dashboard_widget.browserify.js' on my system. That causes a disaster in the pipeline SubProcessCompiler since that expects a tuple. Upon receiving a string it tears it up into hundred individual characters. So instead of

        command = "%s %s %s --deps %s" % (
            getattr(settings, 'PIPELINE_BROWSERIFY_VARS', ''),
            getattr(settings, 'PIPELINE_BROWSERIFY_BINARY', '/usr/bin/env browserify'),
            getattr(settings, 'PIPELINE_BROWSERIFY_ARGUMENTS', ''),
            self.storage.path(infile),
        )

we'd need something like

        command = (
            getattr(settings, 'PIPELINE_BROWSERIFY_BINARY', '/usr/bin/env browserify'),
            getattr(settings, 'PIPELINE_BROWSERIFY_ARGUMENTS', ''),
            "--deps %s" % self.storage.path(infile),
        )

How does this work for others at all?

MrCsabaToth commented 8 years ago

The part which doesn't work well when the passed command is not a tuple but a long string: https://github.com/jazzband/django-pipeline/blob/master/pipeline/compilers/__init__.py#L108

    argument_list = []
    for flattening_arg in command:
        if isinstance(flattening_arg, string_types):
            argument_list.append(flattening_arg)
        else:
            argument_list.extend(flattening_arg)

This would lead to:

CompilerError: [Error 87] The parameter is incorrect
MrCsabaToth commented 8 years ago

Example for tuple compiler command: https://github.com/jazzband/django-pipeline/blob/master/pipeline/compilers/sass.py#L16

MrCsabaToth commented 8 years ago

I'm still doing something wrong, since the compilation errors out though: https://stackoverflow.com/questions/38276360/using-django-pipeline-browserify-on-windows

MrCsabaToth commented 8 years ago

Ooooh, it was changed around in September 2015, before that the long string was valid: https://github.com/jazzband/django-pipeline/commit/1f6b48ae74026a12f955f2f15f9f08823d744515

MrCsabaToth commented 8 years ago

I tried all kinds of combinations to assemble the command. Like leaving the original long string but use split() when passing it to the executor, just like with the https://github.com/j0hnsmith/django-pipeline-browserify/blob/master/pipeline_browserify/compiler.py#L27. My colleague with Mac also couldn't get it to work, so finally we just short circuited the is_outdated to True (plugging in our own compiler).

Something needs to be done about this.

j0hnsmith commented 8 years ago

I haven't used this project since I first created it so I'm not really familiar with how it works now. I've merged a few pull requests which may have had unintended consequences.

When I get a chance I'll setup a dev environment so I try out the latest version of the code.

solkaz commented 8 years ago

@MrCsabaToth @j0hnsmith I've fixed this issue with this PR

j0hnsmith commented 7 years ago

Fixed via #16