Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrapper name doesn't work with cmake #109

Open
froody opened this issue Jul 30, 2024 · 2 comments
Open

Wrapper name doesn't work with cmake #109

froody opened this issue Jul 30, 2024 · 2 comments

Comments

@froody
Copy link

froody commented Jul 30, 2024

I defined cmake_opt, _cmake_opt_internal = with_cfg(cmake).set("compilation_mode", "opt").build() and then declared a rule for proj like so:

package(default_visibility = ["//visibility:public"])

load("@//tools:bazel_files/opt_rules.bzl", "cmake_opt")

filegroup(
    name = "all_srcs",
    srcs = glob(["**"]),
    visibility = ["//visibility:public"],
)

cmake_opt(
    name = "proj",
    cache_entries = {
        "CMAKE_C_FLAGS": "-fPIC",
        "CMAKE_CXX_FLAGS": "-fPIC",
        "BUILD_SHARED_LIBS": "OFF",
        "BUILD_TESTING": "OFF",
        "ENABLE_TIFF": "OFF",

    },
    build_args = [ "-j64" ],
    lib_source = ":all_srcs",
    out_static_libs = ["libproj.a"],
    deps = ["@sqlite3//:sqlite3", "@curl//:curl"],
)

but that gives the following error:

ERROR: /home/tbirch/.cache/bazel/_bazel_tbirch/5dac0c5eac9fcd76e48829b4e488238f/external/proj/BUILD.bazel:12:10: in cmake rule @@proj//:proj_/proj:
Traceback (most recent call last):
	File "/home/tbirch/.cache/bazel/_bazel_tbirch/5dac0c5eac9fcd76e48829b4e488238f/external/rules_foreign_cc/foreign_cc/cmake.bzl", line 190, column 33, in _cmake_impl
		return cc_external_rule_impl(ctx, attrs)
	File "/home/tbirch/.cache/bazel/_bazel_tbirch/5dac0c5eac9fcd76e48829b4e488238f/external/rules_foreign_cc/foreign_cc/private/framework.bzl", line 392, column 30, in cc_external_rule_impl
		outputs = _define_outputs(ctx, attrs, lib_name)
	File "/home/tbirch/.cache/bazel/_bazel_tbirch/5dac0c5eac9fcd76e48829b4e488238f/external/rules_foreign_cc/foreign_cc/private/framework.bzl", line 738, column 21, in _define_outputs
		_check_file_name(lib_name)
	File "/home/tbirch/.cache/bazel/_bazel_tbirch/5dac0c5eac9fcd76e48829b4e488238f/external/rules_foreign_cc/foreign_cc/private/framework.bzl", line 710, column 17, in _check_file_name
		fail("Symbol '%s' is forbidden in library name '%s'." % (letter, var))
Error in fail: Symbol '/' is forbidden in library name 'proj_/proj'.
ERROR: /home/tbirch/.cache/bazel/_bazel_tbirch/5dac0c5eac9fcd76e48829b4e488238f/external/proj/BUILD.bazel:12:10: Analysis of target '@@proj//:proj_/proj' failed

If I apply this diff to with_cfg.bzl, then everything builds fine:

diff --git a/with_cfg/private/wrapper.bzl b/with_cfg/private/wrapper.bzl
index 73fa461..c62cb05 100644
--- a/with_cfg/private/wrapper.bzl
+++ b/with_cfg/private/wrapper.bzl
@@ -66,7 +66,7 @@ def _wrapper(
         original_settings_label,
         attrs_to_reset):
     dirname, separator, basename = name.rpartition("/")
-    original_name = "{dirname}{separator}{basename}_/{basename}".format(
+    original_name = "{dirname}{separator}{basename}_{basename}".format(
         dirname = dirname,
         separator = separator,
         basename = basename,
@fmeum
Copy link
Owner

fmeum commented Jul 30, 2024

That workaround is probably fine for your use case, but it would break other rules: with_cfg uses a subdirectory to have the original target use the same basename as the transitioned target, which can play a role for e.g. shared library names.

Since rules should generally support subdirs in name, the best solution would be to fix this in rules_foreign_cc. I will see whether I can send a PR.

@fmeum
Copy link
Owner

fmeum commented Jul 30, 2024

As a direct workaround, try setting the lib_name attribute explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants