[email protected]

[Ronan0912]

Allow to be added as a system include using angle brackets.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[Ronan0912]

@bazel-io skip_check unstable_url

[bazel-io]

Hello @Attempt3035, modules you maintain (cjson) have been updated in this PR.
Please review the changes. You can view a diff against the previous version in the "Generate module diff" check.

[Ronan0912]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Fixed

[Ronan0912]

Using:

#include <cjson/cJSON.h>

But not:

#include <cJSON.h>

Any idea on how to include with angle brackets without the cjson prefix?

includes = ["."] attribute is refused by CI:

(00:04:07) ERROR: /Users/buildkite/builds/bk-macos-intel-z8jg/bazel-org-repo-root/output/cJSON-12c4bf1986c288950a3d06da757109a6aa1ece38/BUILD.bazel:26:11: in includes attribute of cc_library rule //:cjson: '.' resolves to the workspace root, which would allow this rule and all of its transitive dependents to include any file in your workspace. Please include only what you need. Since this rule was created by the macro 'cc_library', the error might have been caused by the macro implementation

[Attempt3035]

You'll need to add includes = ["cjson"]

Includes basically strips that prefix from the include path

[Ronan0912]

You'll need to add includes = ["cjson"]

Includes basically strips that prefix from the include path

Right, I had to add:

• includes = ["cjson"],
• include_prefix = "cjson",

to pass the CI.

Which then means including as <cjson/cJSON.h>.

[Attempt3035]

Hmm, that should be able to add directly without the cjson folder prefix. Are you sure the compiler isn't picking up a system install of cjson? (as angled brackets look for system install before project dependency install)

[Attempt3035]

Oh, you might need to add "cjson/" as the strip prefix

[Ronan0912]

Hmm, that should be able to add directly without the cjson folder prefix. Are you sure the compiler isn't picking up a system install of cjson? (as angled brackets look for system install before project dependency install)

From my experiment:

includes = [.] => -isystem external/cjson+

includes = ["cjson"] => -isystem external/cjson+/cjson

Oh, you might need to add "cjson/" as the strip prefix

If putting cjson for both the includes and strip_include_prefix then I get the following error:

Error in fail: header 'cJSON.h' is not under the specified strip prefix 'cjson'

[Ronan0912]

Without moving the cJSON.h to an include directory I don't see much we can do (cjson/ prefix is an acceptable tradeoff)

[Attempt3035]

Just to clarify, the goal is to be able to import cjson as #include "cjson.h" OR #include <cjson.h> ?

In which case yes just the strip prefix cjson/ should be all we need, not the includes

Error in fail: header 'cJSON.h' is not under the specified strip prefix 'cjson'

Yes, sorry I wasn't clear with this, using includes exposes the files in that folder directly, strip prefix just strips the leading path of the header files. Nearly equivalent, but as you saw there's slight differences with how Basel works with system style imports between these two methods. Either way we can't do both at the same time

[Ronan0912]

Just to clarify, the goal is to be able to import cjson as #include "cjson.h" OR #include <cjson.h> ?

The goal is to do: #include <cJSON.h> (and also #include "cJSON.h" if wanted)

There is no cjson folder and cJSON.h/c are directly at the root directory.

Yes, sorry I wasn't clear with this, using includes exposes the files in that folder directly, strip prefix just strips the leading path of the header files. Nearly equivalent, but as you saw there's slight differences with how Basel works with system style imports between these two methods. Either way we can't do both at the same time

I also though strip_include_prefix = "." would work but it looks not.

[Ronan0912]

Fixed by adding a patch to move all headers to an include directory.

[Ronan0912]

Fixed by adding a patch to move all headers to an include directory.

Actually copying all headers because some tests are including directly as #include "../cJSON_Utils.h"

[Attempt3035]

Ahh, sorry I misunderstood part of the problem. Just adding:
includes = ["."],

As you mentioned earlier did indeed work for me, as the compiler now indeed falls back to searching included folders if failing system libs (allows adding via both #include <cJSON.h> and #include "cJSON.h", tested on MacOS with Clang) does it solve it for you?

(It should be functionally equivalent to the copy to include + includes=["include"] method) - That way we can avoid duplicating headers to another folder.

[Ronan0912]

Indeed just adding includes = ["."] on my ubuntu 22.04 and it was the first commit I made:

bfa582a

But the BCR presubmit was failing when running the test module:

https://buildkite.com/bazel/bcr-presubmit/builds/14237#0196f4de-6bff-4deb-ab0c-da0751b7ec4c

(22:01:36) ERROR: /var/lib/buildkite-agent/builds/bk-docker-vb5h/bazel-org-repo-root/output/cJSON-12c4bf1986c288950a3d06da757109a6aa1ece38/BUILD.bazel:26:11: in includes attribute of cc_library rule //:cjson: '.' resolves to the workspace root, which would allow this rule and all of its transitive dependents to include any file in your workspace. Please include only what you need. Since this rule was created by the macro 'cc_library', the error might have been caused by the macro implementation

[Attempt3035]

Got it. Let's use a genrule to do the file copy, so that we don't need to maintain copy patches (as when the lib is updated we'd have to re-make the patches every time we bump the source version)

Try:

cc_library(
    name = "cjson",
    srcs = ["cJSON.c"],
    hdrs = [":copy_headers"],
    includes = ["include"],
    copts = CJSON_COPTS + [
        "-DCJSON_EXPORT_SYMBOLS",
        "-DCJSON_API_VISIBILITY",
    ],
    linkopts = select({
        "@bazel_tools//src/conditions:windows": [],
        "//conditions:default": ["-lm"],  # Link math library on non-Windows
    }),
    visibility = ["//visibility:public"],
)

genrule(
    name = "copy_headers",
    srcs = ["cJSON.h"],
    outs = ["include/cJSON.h"],
    cmd = "mkdir -p $(@D) && cp $(SRCS) $(@D)/cJSON.h",
)

Works and passes all tests on my computer, it should work for windows too but if the BCR windows builds fail lmk and we can add a windows specific copy command

[Attempt3035]

My bad, try having the genrule as a dep and the header referenced directly:

cc_library(
    name = "cjson",
    srcs = ["cJSON.c"],
    hdrs = ["include/cJSON.h"],
    deps = [":copy_headers"],
    includes = ["include"],
    copts = CJSON_COPTS + [
        "-DCJSON_EXPORT_SYMBOLS",
        "-DCJSON_API_VISIBILITY",
    ],
    linkopts = select({
        "@bazel_tools//src/conditions:windows": [],
        "//conditions:default": ["-lm"],  # Link math library on non-Windows
    }),
    visibility = ["//visibility:public"],
)

genrule(
    name = "copy_headers",
    srcs = ["cJSON.h"],
    outs = ["include/cJSON.h"],
    cmd = "mkdir -p $(@D) && cp $(SRCS) $(@D)/cJSON.h",
)

If that doesn't fix it, the cmd might be silently failing or misplacing the file on windows

[Ronan0912]

Right!

I removed the mkdir as it is automatically done by Bazel. We could also have used the copy_file rule from skylib but it would add a dependency:

https://github.com/bazelbuild/bazel-skylib/blob/main/rules/private/copy_file_private.bzl

genrule(
   name = "copy_header",
   srcs = ["cJSON.h"],
   outs = ["include/cJSON.h"],
   cmd = "cp $(SRCS) $(@)",
)

cc_library(
   name = "cjson",
   srcs = ["cJSON.c"],
   hdrs = ["include/cJSON.h"],
   deps = [":copy_header"],
   includes = ["include"],
   copts = CJSON_COPTS + [
       "-DCJSON_EXPORT_SYMBOLS",
       "-DCJSON_API_VISIBILITY",
   ],
   linkopts = select({
       "@bazel_tools//src/conditions:windows": [],
       "//conditions:default": ["-lm"],  # Link math library on non-Windows
   }),
   visibility = ["//visibility:public"],
)

[Ronan0912]

@Attempt3035 All tests are passing so ready to merge

[Attempt3035]

Looks perfect! Thanks for this!! 👌

[bazel-io]

Hello @bazelbuild/bcr-maintainers, all modules in this PR have been approved by their maintainers. This PR will be merged if all presubmit checks pass.