[native_toolchain_c] Defines with quotation marks (") break build process on Windows

[zeyus]

Using CBuilder.library, on Windows, the build crashes with:

'C:\Program' is not recognized as an internal or external command, operable program or batch file.

Error: Compiling native assets failed.

This happens because the escaping / string quoting for defines, PATH, other environment variables fails when a define has a double quotation mark in it (it's possible this will apply to flags as well)

import 'package:logging/logging.dart';
import 'package:native_assets_cli/code_assets.dart';
import 'package:native_toolchain_c/native_toolchain_c.dart';

void main(List<String> args) async {
  final packageName = input.packageName;
  await build(args, (input, output) async {
    var defines = <String, String>{
      'WORKING_DEFINE': "'VAL'",
      'BROKEN_DEFINE': '"VAL"',
    };

    final builder = CBuilder.library(
      name: packageName,
      assetName: '$packageName.dart',
      pic: true,
      std: 'c++17',
      sources: [
         // ...
      ],
      language: Language.cpp,
      includes: [
        // ...
      ],
      defines: defines,
    );

    await builder.run(
        input: input,
        output: output,
        logger: Logger('')
          ..level = Level.ALL
          ..onRecord.listen((record) => print(record.message)));
  });
}

[dcharkes]

Hi @zeyus! Thanks for the report!

This should be an issue in package:native_toolchain_c.

We're open to contributions, would you take a stab at fixing the package?

[zeyus]

Hi @zeyus! Thanks for the report!

This should be an issue in package:native_toolchain_c.

We're open to contributions, would you take a stab at fixing the package?

Hey @dcharkes thanks, sorry for the wrong package category. And yes, I'm happy to have a look and submit a PR.

[zeyus]

@dcharkes PR submitted, it's a bit weirder than I hoped, but it works

[dcharkes]

Thanks for the great documentation on the PR! TIL about /FI.

I wonder if it would be cleaner to (1) add forceIncludes param to the CBuilder and (2) give ArgumentError on Windows for having quotes in the parameters. This would keep CBuilder as thin as a skin around compilers as possible.

For (1), the clang equivalent is https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gcc/Preprocessor-Options.html#index-include apparently.

For (2), we should probably point the new forceIncludes param. 😄

WDYT @zeyus?

[zeyus]

@dcharkes Yeah that makes sense, if it's an option that can be exposed across compilers (I didn't know about the clang version), then it also prevents potential confusion as to what's happening under the hood (even if it shouldn't have too much of an impact).

I see below the option for (1) there's also -imacros file which only processes macros and ignores code, but if the best way is to expose a new param, then it should be as similar in behaviour across compilers (because /FI includes macros and code).

I think this is better way. I didn't consider initially because I was trying to touch as little of the code as possible, but I can update the PR later using this approach. I should be able to run most/all of the test variants too.

---

The whole reason I even came across this was because I've made a dart package for liblsl / Lab streaming layer (and even though I'm just really targeting iOS for my experiment, I wanted it to be useful for any targets), and the liblsl source has one weird line where they include the version string assigned to a const but it uses the macro definition which has to include the quotes, otherwise the assignment fails.

On that, I have been reading a bit about WASM and its status in dart for web targets, and it seems a shame if I couldn't get liblsl working for web as well, but at least as I understand it, probably isn't and won't be possible without essentially re-writing or reimplementing it specifically for browsers? I think most of the core functionality would be supported but that's beyond my current level.

[dcharkes]

I see below the option for (1) there's also -imacros file which only processes macros and ignores code, but if the best way is to expose a new param, then it should be as similar in behaviour across compilers (because /FI includes macros and code).

Yes lets do the same behavior across compilers. 👍

where they include the version string assigned to a const but it uses the macro definition which has to include the quotes, otherwise the assignment fails.

Haha, the world out there is full of quirky details. 😄

RE wasm: In some future we'd like to have build hooks that can do dylibs and WASM. And possibly have a WasmOrCBuilder that could wrap both. There is much engineering work to be done before we're there. If you're interested, that work can be tracked in:

• Compilation of native code to WASM #156
• [native_assets_cli] build.dart / link.dart web backends and WasmCode and JsCode assets #988

[zeyus]

@dcharkes PR updated with new arg