@error is non fatal (should be fatal, is inconsistent with custom functions)

[asottile]

I'm running against current master: c15b384

The following:

#include "sass_context.h"
#include <cstring>
#include <iostream>


int main() {
    // ??? in 3.1.0 I could just call sass_make_data_context("a literal")
    // now I need to put it on the heap? and `free`ing it causes double free?
    // (unrelated but made it harder to make this bugreport)
    char* src = (char*)malloc(sizeof(char) * 50);
    strcpy(src, "a { @error 'foo'; }");
    struct Sass_Data_Context* context = sass_make_data_context(src);
    sass_compile_data_context(context);
    struct Sass_Context* ctx = sass_data_context_get_context(context);
    std::cout << "Error status:" << std::endl;
    std::cout << sass_context_get_error_status(ctx) << std::endl;
    sass_delete_data_context(context);

    return 0;
}

Produces:

$ ./main 
Error: foo
    on line 0 of stdin

Error status:
0

However this code (using custom functions):

#include "sass_context.h"
#include <cstring>
#include <iostream>


static union Sass_Value* custom_func(const union Sass_Value* args, void* cookie) {
    return sass_make_error("hello world");
}


int main() {
    char* src = (char*)malloc(sizeof(char) * 50);
    strcpy(src, "a { content: foo(); }");
    struct Sass_Data_Context* context = sass_make_data_context(src);
    struct Sass_Options* options = sass_data_context_get_options(context);
    Sass_C_Function_List fn_list = sass_make_function_list(1);
    Sass_C_Function_Callback fn = sass_make_function("foo()", custom_func, NULL);
    sass_function_set_list_entry(fn_list, 0, fn);
    sass_option_set_c_functions(options, fn_list);
    sass_compile_data_context(context);
    struct Sass_Context* ctx = sass_data_context_get_context(context);
    std::cout << "Error status:" << std::endl;
    std::cout << sass_context_get_error_status(ctx) << std::endl;
    sass_delete_data_context(context);

    return 0;
}

Produces:

$ ./main_custom_func 
Error status:
1

The same is reproduced for @warn as well (fatal in custom functions, nonfatal outside).

Prior art:

• ruby sass produces a failure for @error nodes and does not fail on @warning nodes

[xzyfer]

This should probably be fixed for 3.2.0. Is that possible @mgreter ?

[mgreter]

I need at least a sample SCSS code, OS and compiler version to debug this any further! To be honest, I don't even get OPs problem, since without returning an error he seems to get Error status: 0 and when returning an error you get Error status: 1. Doesn't that be what you'd expect. Maybe I haven't understood the problem actually!

[xzyfer]

If I'm understanding correctly.

Executing the following via the C API produced the expected Sass @error but sass_context_get_error_status returns 0.

a { @error 'foo'; }

However creating a custom function which throws a sass_make_errornow sass_data_context_get_context returns 1.

a { content: foo(); } // foo is a custom function;

It appears that @error does not return a valid error exit code. I can reproduce this sassc as well.

$ sass test.scss
Error: bar
        on line 2 of test.scss
  Use --trace for backtrace.
$ echo $?
65
$ sassc test.scss
Error: bar
    on line 1 of test.scss

$ echo $?
0

[mgreter]

Ah, now I understand, but I wonder if this is the expected behavior. I guess this also should be part of our roadmap to support error spec tests (we should somehow include the expected exit status there). Just to let everyone know, mingw has a problem when it throws native errors (like runtime_exception), which is a known bug and will not be catched by any implementor (when compiled with mingw!). Actually the fix for this seems pretty easy, as we only would need to call exit in custom errors explicitly!

IMO this is a pretty minor bug and I'm not sure if we can address this in 3.2 beta.
BTW. both features are still marked as experimental (mainly importers and less functions)!

[asottile]

This is pretty important for our build as we use @error as a way to fatally terminate compilation. Silently printing and not indicating there is an error is not really going to work for us (and is pretty unfortunate behavior!)

[xzyfer]

I agree with @asottile this is really important. People using Sassc in their CI environments need to know if their Sass failed to compile. That almost always done by looking for non-zero exit code. I don't think the code itself matters, as long as it's non-zero.

[xzyfer]

At the very least the sassc example I showed is critical IMO.

[mgreter]

I ACK that this is probably not working as expected, but as I said, you're using experimental features here. In order to be able to stabilize such features, we need users like you that run into such traps we didn't anticipate! IMO we have more important regressions to address in this beta and I'm pretty sure this bug was existing since its introduction. I hope this doesn't offend you, since it only means a re-scoping for that issue. And by your original examples, I would guess you have a pretty good knowledge of c/c++! So would like to encourage you to take look at the code, maybe you're able to solve you're problem on your own and provide a PR to libsass!

[mgreter]

@xzyfer we are IMO only talking about returning errors from custom functions! So this should not touch sassc by any means!

[xzyfer]

Sorry for not being clear, this is a problem in sassc with the latest Libsass.

foo {
  @error("bar");
}

$ sass test.scss
Error: bar
        on line 2 of test.scss
  Use --trace for backtrace.
$ echo $?
65
$ sassc test.scss
Error: bar
    on line 1 of test.scss

$ echo $?
0

Maybe it's a separate issue, but at the very least I see this use case as critical.

[mgreter]

You're sure that wasn't the same before?
(Just want to know if it is a bug or regression?)

[xzyfer]

Looks like it's always been that way, but @error was only introduced in the last version.

[mgreter]

@xzyfer you mean for custom functions, or by actual "first appearance"?

[xzyfer]

The sass @error directive was introduced in 3.1.0. It never returned the correct (non-zero) error code. That's the bug I'm concerned about.

As you said custom functions are experimental so I'm not concerned. SassC however isn't.

[mgreter]

OK, got it now :) IMO let's first asses how difficult it is to fix, before adding a milestone again! But yeah, it should be a pretty trivial fix in this case 😉 (error status is set in sass_context ...).

[xzyfer]

IMO we should aim to get this in 3.2.0. Without it SassC is pretty much useless for companies with CI environments, because there's not clear way to determine if compilation failed.

But yes, if it's too difficult to fix we'll have to figure out a way forward for SassC at least.

[mgreter]

I do agree, but to be honest I would expect a company to have one guy submitting a PR to solve their problem. IMO we now have a very active merge policy, so we should encourage participation by companies in exactly this way. That's probably my personal opinion, but if companies want to use this software, they should at least be willing to contribute a little back; If they encounter a problem they can solve in one or two days, they should just do that and submit a PR!

[asottile]

Sure sure, I'm going to try and look into this myself as well :)

[mgreter]

👍 that's great news! And if you need any further help, feel free to ask. I would start somewhere in sass_context, since this is the first layer which sassc interacts with and where the actual error status should be passed to sassc (aka the implementor)!

[asottile]

Yep I'm pretty familiar with the interface ( I implemented python bindings with custom functions for 3.1.0 here: https://github.com/dahlia/libsass-python/pull/44/files ). I'll hopefully have something by end of tonight.

[asottile]

How should I regression test this?

[mgreter]

To be honest, manually! We don't have any other means to test this currently (and exit code probably is second prio after beeing able to spec test error messages on stderr). The only thing I could offer is that you put your test into perl-libsass.