bazel-central-registry icon indicating copy to clipboard operation
bazel-central-registry copied to clipboard

add fallback curl config

Open zaucy opened this issue 2 months ago • 9 comments

several C/C++ packages define HAVE_CONFIG_H which leaks into curl causing an error

this adds the default curl_config.h header to avoid this issue

alternatively we could update several other packages in the BCR to not define HAVE_CONFIG_H

zaucy avatar Apr 30 '24 20:04 zaucy

Hello @keith, modules you maintain (curl) have been updated in this PR. Please review the changes.

bazel-io avatar Apr 30 '24 20:04 bazel-io

wouldn't it have to be patches that are in the curl dependency tree and setting that as defines to cause this issue?

keith avatar May 03 '24 17:05 keith

I don't think so. I believe if you have a target that depends on @curl and some other package that defines HAVE_CONFIG_H then it will leak to curls compilation.

from bazel docs about defines

[...] added to the compile command line to this target, as well as to every rule that depends on it. Be very careful, since this may have far-reaching effects.

This goes for copts as well, I believe.

zaucy avatar May 03 '24 17:05 zaucy

these are the modules that currently set HAVE_CONFIG_H in the bcr which I think would affect the curl module if compiled together:

  • c-ares
  • libarchive
  • xz
  • ncurses
  • nasm
  • snappy

In my project I believe mine was caused by utilising curl and libarchive. I only tested on Windows using msvc though.

zaucy avatar May 03 '24 17:05 zaucy

as well as to every rule that depends on it

meaning curl would have to depend on the libraries that have it for it to cause an issue.

note that I'm actually working on making curl use HAVE_CONFIG_H which i think is safer than what we're doing now

keith avatar May 03 '24 22:05 keith

should be fixed by https://github.com/bazelbuild/bazel-central-registry/pull/1943

keith avatar May 03 '24 23:05 keith

meaning curl would have to depend on the libraries that have it for it to cause an issue.

I see what you're saying, but I assumed the opposite was true as well. I've been able to reproduce it here: https://github.com/ecsact-dev/ecsact_cli/actions/runs/8947494955/job/24579495333#step:4:270

cc_binary(
    name = "ecsact_cli",
    # ...
    deps = [
        "//ecsact/cli/commands:codegen",
        "//ecsact/cli/commands:command",
        "//ecsact/cli/commands:config",
        "//ecsact/cli/commands:build",
        "//ecsact/cli/commands:recipe-bundle", # offending target with @curl + @libarchive
    ],
)
ERROR: D:/a/ecsact_cli/ecsact_cli/ecsact/cli/commands/recipe-bundle/BUILD.bazel:6:11: Compiling ecsact/cli/commands/recipe-bundle/build_recipe_bundle.cc failed: (Exit 2): cl.exe failed: error executing CppCompile command (from target //ecsact/cli/commands/recipe-bundle:build_recipe_bundle) 
C:\Users\runneradmin\_bazel_runneradmin\uybixras\execroot\_main\external\curl~\lib\curl_setup.h(82): fatal error C1083: Cannot open include file: 'curl_config.h': No such file or directory

When I comment out @libarchive//libarchive in the target that uses @curl (and remove the libarchive related code) I compile successfully. I can't help but think that the HAVE_CONFIG_H is leaking directly or indirectly due to @libarchive//libarchive 🤔

I'm trying to make a smaller repro, but I haven't successfully done so.

zaucy avatar May 04 '24 03:05 zaucy

interesting. as long as HAVE_CONFIG_H is still in the local_defines it shouldn't leak into curl there.

keith avatar May 06 '24 20:05 keith

several packages have it in defines or copts which I believe is the issue, but not all of them can use local_defines since the config header is used in public headers.

zaucy avatar May 08 '24 17:05 zaucy