rules_nixpkgs icon indicating copy to clipboard operation
rules_nixpkgs copied to clipboard

Fix serialization in toolchain file

Open hofbi opened this issue 1 year ago • 5 comments

Used the wrong serialization in https://github.com/tweag/rules_nixpkgs/pull/575. This should fix it to not end up in

extra_flags_per_feature = %{extra_flags_per_feature}

in the generated BUILD file

hofbi avatar Aug 26 '24 15:08 hofbi

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 []

jtszalay avatar Aug 26 '24 18:08 jtszalay

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?

aherrmann avatar Aug 27 '24 07:08 aherrmann

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.

hofbi avatar Aug 27 '24 08:08 hofbi

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.

avdv avatar Aug 27 '24 11:08 avdv

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.

aherrmann avatar Aug 27 '24 11:08 aherrmann

@mergifyio update

hofbi avatar Sep 04 '24 10:09 hofbi

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

mergify[bot] avatar Sep 04 '24 10:09 mergify[bot]