Improve importer C-API and path handling

[mgreter]

This is a breaking C-API change, altough only minor and in an experimental feature!

Renamed two API functions to be more close to their actual use:
sass_import_get_path -> sass_import_get_imp_path
sass_import_get_base -> sass_import_get_abs_path

This PR also reworks and should make path handling better understandable. Still far from perfect under the hood, but IMO the API is slowly getting there. It should now support urls correctly.

This also fixes another bug introduced by a fix commited a few days ago. Headers were all stored under the same virtual file-name. Therefore the "cache" always returned the same header. We now add an index to the virtual file-name if multiple headers are included in the same file (Fixes #1505).

[drewwells]

Does this break any existing functions or have just these two functions
moved to new names?
On Mon, Aug 31, 2015 at 6:11 PM Marcel Greter [email protected]
wrote:

This is a breaking C-API change, altough only minor and in an experimental
feature!

Renamed two API functions to be more close to their actual use:
sass_import_get_path -> sass_import_get_imp_path
sass_import_get_base -> sass_import_get_abs_path

This PR also reworks and should make path handling better understandable.
Still far from perfect under the hood, the IMO the API is slowly getting
there. It should now support urls correctly.

This also fixes another bug introduces by a fix commited a few days ago.
Headers were all stored under the same virtual file-name. Therefore the
"cache" always returned the same header. We now add an index to the virtual

file-name if multiple headers are included in the same file.

You can view, comment on, or merge this pull request online at:

#1509
Commit Summary

• Improve importer C-API and path handling

File Changes

• M include/sass_functions.h
  https://github.com/sass/libsass/pull/1509/files#diff-0 (6)
• M src/context.hpp
  https://github.com/sass/libsass/pull/1509/files#diff-1 (4)
• M src/parser.cpp
  https://github.com/sass/libsass/pull/1509/files#diff-2 (28)
• M src/sass_functions.cpp
  https://github.com/sass/libsass/pull/1509/files#diff-3 (18)

Patch Links:

• https://github.com/sass/libsass/pull/1509.patch
• https://github.com/sass/libsass/pull/1509.diff

—
Reply to this email directly or view it on GitHub
#1509.

[mgreter]

There were some changes under the hood to support urls better. Other than that everything should behave as before (perl-libsass unit tests also triggered and are now working again).

[saper]

(related: #1160)

[xzyfer]

No objections from me.

[drewwells]

@mgreter most of my header tests are passing now.

I found one last oddity. If you include a header then do a file compile, the only file that shows up in resolved imports is the header not the file being compiled. Previously, the resolved imports included only the file being compiled and not the header. If I remove the header and do a file compile, the resolved imports includes an absolute path to the file being compiled.

Input

import_entry_....("/path/to/header");
sass_make_file_context("/path/to");

Previous:

resolved_imports -> "/path/to";

Now:

resolved_imports -> "header"

Imo, correct:

resolved_imports -> ["/abs/path/to/header", "/abs/path/to/file"]

[mgreter]

What is resolved_imports? That's not from libsass directly!? Are you sure you use abs_path and not imp_path? Can you reproduce with a plain C sample!?

[drewwells]

sass_context_get_included_files. Yeah I switched to abs_path/imp_path, it
didn't compile until I did that :).

On Tue, Sep 1, 2015 at 2:50 AM Marcel Greter [email protected]
wrote:

What is resolved_imports? That's not from libsass directly!? Are you sure
you use abs_path and not imp_path? Can you reproduce with a plain C
sample!?

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

[drewwells]

This shows what I'm talking about

#include <stdio.h>
#include <string.h>
#include "sass_context.h"

Sass_Import_List sass_importer(const char* path, Sass_Importer_Entry cb, struct Sass_Compiler* comp)
{
  // get the cookie from importer descriptor
  void* cookie = sass_importer_get_cookie(cb);
  Sass_Import_List list = sass_make_import_list(2);
  const char* local = "local { color: green; }";
  const char* remote = "remote { color: red; }";
  list[0] = sass_make_import_entry("/tmp/styles.scss", strdup(local), 0);
  list[1] = sass_make_import_entry("http://www.example.com", strdup(remote), 0);
  return list;
}

int main( int argc, const char* argv[] )
{

  // get the input file from first argument or use default
  const char* input = argc > 1 ? argv[1] : "../test/scss/main.scss";

  // create the file context and get all related structs
  struct Sass_File_Context* file_ctx = sass_make_file_context(input);
  struct Sass_Context* ctx = sass_file_context_get_context(file_ctx);
  struct Sass_Options* ctx_opt = sass_context_get_options(ctx);

  // allocate custom importer
  Sass_Importer_Entry c_imp =
    sass_make_importer(sass_importer, 0, 0);
  // create list for all custom importers
  Sass_Importer_List imp_list = sass_make_importer_list(1);
  // put the only importer on to the list
  sass_importer_set_list_entry(imp_list, 0, c_imp);
  // register list on to the context options
  sass_option_set_c_headers(ctx_opt, imp_list);
  // context is set up, call the compile step now
  int status = sass_compile_file_context(file_ctx);

  // print the result or the error to the stdout
  if (status == 0) puts(sass_context_get_output_string(ctx));
  else puts(sass_context_get_error_message(ctx));

  char** imps = sass_context_get_included_files(ctx);
  size_t size = sass_context_get_included_files_size(ctx);
  for (int i=0; i<size; i++) {
    printf("%s\n", imps[i]);
  }

  // release allocated memory
  sass_delete_file_context(file_ctx);

  // exit status
  return status;

}

Output:
//www.example.com

Should be:

//www.example.com
/abs/test/scss/main.scss

[mgreter]

You're case should be covered now. Please open new bugs if I broke anything else!

[drewwells]

So is it considered a bug that the headers are still left out of imports on the file compiler?

[mgreter]

This is by design and I would argue that it doesn't make much sense to include them there (it is included in the source-maps though, as you can see when you enable "Custom Header Test" in srcmap inspector). I guess @import statements in the header should be included but not the headers themselve. Most implementors use IMO that array to get info which files they need to watch and headers should be generated by code, so they cannot be watched. If they actually can, you should probably just use an import (either in the headers or in the main source file). Does that make sense?

[drewwells]

That is fine, I don't have a use case for watching headers just noticed that the unit tests are failing. It might be worth documenting somewhere that headers are not included in the resolved imports list. I did notice these before so that it would be a breaking change from 3.2.5 behavior.