Fix sass_option_push_include_path / sass_option_push_plugin_path #1974
Conversation
| collect_include_paths(paths_array[i]); | ||
| } | ||
|
|
||
| while (paths_array) |
There was a problem hiding this comment.
Could be that you're using tabs?
xoofx
force-pushed
the
fix_sass_option_push_include_path
branch
from
e676e1a
to
158f144
Compare
No, I did a fix and push force in the meantime ;) You can see above a PR for sassc using this API. |
|
I think there was a compelling reason I disabled these, but I can't remember exactly what it was. @xoofx can you say how widely you tested it? We need to make sure it works with both APIs and preferably doesn't break if someone is funky enough to use both (IMO that we have a fixed order here is fine as doing so should not be encouraged anyway). Other than that LGTM! |
|
We have scheduled breaking API changes for 4.0 so I'm 👍 to shipping this and getting userland feedback. |
|
I'm going to be AFK for the next week. @mgreter please ship this if you have no objections. |
|
I don't even consider this one a breaking change but additional feature? But need to test this first. I hope I have some unit tests in perl-libsass for this. Maybe you can see and give feedback if node-sass is happy with the changes? BTW. I was under the impression we make another iteration with the C-API in 3.4 (which might could be a rc for 4.0 IMO). I also would like to get rid of the obsolete C interface in 3.4. But there are a few todos open before that (access to current file, import stack etc.). |
|
I won't have time this week, maybe @saper will. The SassC change looks promising (https://github.com/sass/sassc/pull/166/files) |
|
@mgreter I tested it only on my wrapper and in the sassc branch. I have checked that using |
| @@ -185,11 +185,14 @@ namespace Sass { | |||
| } | |||
| } | |||
|
|
|||
| void Context::collect_include_paths(const char** paths_array) | |||
| void Context::collect_include_paths(string_list* paths_array) | |||
| { | |||
| if (!paths_array) return; | |||
There was a problem hiding this comment.
The if would no longer be needed (same while clause below)
xoofx
force-pushed
the
fix_sass_option_push_include_path
branch
from
158f144
to
7b9065f
Compare
| // free intermediate data | ||
| free(include_paths); | ||
| free(plugin_paths); | ||
|
|
There was a problem hiding this comment.
Maybe you konw/can explain why this is no longer needed?
I'm also a bit worried about #1871 (comment)
Last check should be if there are any memory leaks.
But it doesn't break perl-libsass, so LGTM so far.
There was a problem hiding this comment.
Well, the previous code was basically a no-op (filling include_paths and then after freeing it).
That being said, there is a slight issue with c_ctx->include_paths that will not be released.
But I'm really not enough sure how ownership is currently working in libsass, if you could enlighten me. Looking at the code, I would assume that include_paths should be free in the sass_clear_context. Is it correct?
There was a problem hiding this comment.
ah, actually, https://github.com/sass/libsass/blob/master/src/sass_context.cpp#L527 seems to release things, should be ok then? 😅
There was a problem hiding this comment.
Well there is std::vector<std::string> plugin_paths in context.hpp and struct string_list* plugin_paths in sass_context.hpp. But yes, normally we can expect memory to be alive until the main context is freed. There were also a lot of refactorings in the C++ part to re-use more of the C structs directly.
|
Sorry to be a bit nitpicky @xoofx. Any chance you have made an "end to end" test with sassc that requres the include path via cmd line? |
|
I have tested it on the command line (only on Windows), so yes, I made a test, though quite basic. I was assuming also that there are some unit test using it? |
|
@xoofx can you rebase this to latest master so CI passes. Thanks! |
… were not working
xoofx
force-pushed
the
fix_sass_option_push_include_path
branch
from
7b9065f
to
bf471e2
Compare
done |
|
@mgreter I've updated node-sass to LibSass master + this patch. Test pass. I am not using the updated interface however because include paths as a delimited string is much cleaner for us. |
|
I'm happy to merge this and ship 3.3.5. |
This is a fix for #1871
Trying to use
sass_option_push_include_path/sass_option_push_plugin_pathI discovered that they were completely unplugged, while these APIs are useful to push path when the list is already prepared on the user side and we don't want to concatenate the includes (that will be splitted anyway by libsass)