Support empty extra_flags_per_feature

[hofbi]

Used the wrong serialization in #575. Since this is not really used yet, we set always set an empty dict for now.

Closes #578

[jtszalay]

I tried to use this (as I was still getting an error after PR #575:

ERROR: ../bazel-user-root/output/external/nixpkgs_config_cc/BUILD.bazel:154:31: syntax error at ',': expected expression
ERROR: ../bazel-user-root/output/external/nixpkgs_config_cc/BUILD.bazel: no such target '@@nixpkgs_config_cc//:cc-compiler-k8': target 'cc-compiler-k8' not declared in package '' defined by ../bazel-user-root/output/external/nixpkgs_config_cc/BUILD.bazel
ERROR: ../bazel-user-root/output/external/rules_go~/BUILD.bazel:86:17: Target '@@rules_go~//:cgo_context_data' depends on toolchain '@@nixpkgs_config_cc//:cc-compiler-k8', which cannot be found: no such target '@@nixpkgs_config_cc//:cc-compiler-k8': target 'cc-compiler-k8' not declared in package '' defined by ../bazel-user-root/output/external/nixpkgs_config_cc/BUILD.bazel'

It's correct that my extra flags get gen'd as:

    extra_flags_per_feature = ,
)

Previously they were:

extra_flags_per_feature = [],

ERROR: ../bazel-user-root//output/external/nixpkgs_config_cc/BUILD.bazel:92:20: @@nixpkgs_config_cc//:local: expected value of type 'dict(string, list(string))' for attribute 'extra_flags_per_feature' in 'cc_toolchain_config' rule, but got [] (list)
ERROR: ../bazel-user-root/output/external/rules_go~/BUILD.bazel:86:17: Target '@@rules_go~//:cgo_context_data' depends on toolchain '@@nixpkgs_config_cc//:cc-compiler-k8', which cannot be found: Target '@@nixpkgs_config_cc//:cc-compiler-k8' contains an error and its package is in error'

It seems to be expecting something of shape {} not []

[aherrmann]

Interesting, the template built into Bazel includes this

    extra_flags_per_feature = %{extra_flags_per_feature},

other fields that should be set via get_starlark_list look like this instead

    coverage_link_flags = [%{coverage_link_flags}],

FWIW, Bazel itself also uses repr. @hofbi In which way does it fail without this PR?

[hofbi]

Without this PR, I get the same error as reported by @jtszalay

ERROR: ../bazel-user-root/output/external/nixpkgs_config_cc/BUILD.bazel:154:31: syntax error at ',': expected expression
ERROR: ../bazel-user-root/output/external/nixpkgs_config_cc/BUILD.bazel: no such target '@@nixpkgs_config_cc//:cc-compiler-k8': target 'cc-compiler-k8' not declared in package '' defined by ../bazel-user-root/output/external/nixpkgs_config_cc/BUILD.bazel
ERROR: ../bazel-user-root/output/external/rules_go~/BUILD.bazel:86:17: Target '@@rules_go~//:cgo_context_data' depends on toolchain '@@nixpkgs_config_cc//:cc-compiler-k8', which cannot be found: no such target '@@nixpkgs_config_cc//:cc-compiler-k8': target 'cc-compiler-k8' not declared in package '' defined by ../bazel-user-root/output/external/nixpkgs_config_cc/BUILD.bazel'

I will try to fully reproduce and test with a local_override but might only get to to next week.

[avdv]

Interesting, the template built into Bazel includes this

    extra_flags_per_feature = %{extra_flags_per_feature},

Yes, but In Bazel, the extra_flags_per_feature variable is a dict (see https://github.com/bazelbuild/bazel/blob/e88a7d982411e3d6bbfb658b88e76944ea5ed720/tools/cpp/unix_cc_configure.bzl#L559C5-L559C33) and using repr is just writing that dict to the BUILD file.

So, the EXTRA_FLAGS_PER_FEATURE variable in cc.nix should be an associative array and be written to the CC_TOOLCHAIN_INFO file in a form that makes it possible to decode it as a dict again. Arguably, since the variable never has a meaningful value currently, always writing {} would be the easiest thing to fix this.

[aherrmann]

Thank you @avdv, good catch!

Arguably, since the variable never has a meaningful value currently, always writing {} would be the easiest thing to fix this.

Yes, that's an easy quick fix.

So, the EXTRA_FLAGS_PER_FEATURE variable in cc.nix should be an associative array and be written to the CC_TOOLCHAIN_INFO file in a form that makes it possible to decode it as a dict again.

This may be a good motivation to finally replace the CC_TOOLCHAIN_INFO format by json. This format was introduced before Bazel gained json.decode.

[avdv]

dict() expects an iterable of pairs as its first argument. info.extra_flags_per_feature is a list of strings. This will only work out if this list is empty. So, this is a bit complicated way of always writing out {}.

Is there a way forward for the EXTRA_FLAGS_PER_FEATURE in cc.nix to be useful? Otherwise I'd rather remove it until there is a use case for it.

[hofbi]

Would you suggest we simply do

Suggested change

	"%{extra_flags_per_feature}": repr(dict(info.extra_flags_per_feature)),
	"%{extra_flags_per_feature}": repr({}),

and remove the rest until there is a concrete usecase?

[avdv]

Yes, even just using "{}" would do.

[hofbi]

Done

[avdv]

Thank you @hofbi , LGTM!

[hofbi]

@Mergifyio update

[mergify]

update

❌ Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/workflow.yaml without workflows permission