Skip to content

Fix pic default for musl#668

Draft
keith wants to merge 2 commits into
mainfrom
ks/fix-pic-default-for-musl
Draft

Fix pic default for musl#668
keith wants to merge 2 commits into
mainfrom
ks/fix-pic-default-for-musl

Conversation

@keith

@keith keith commented Jul 2, 2026

Copy link
Copy Markdown
Member

Previously since this defaulted to on, linking cgo with musl would fail
because cgo assumes pic is off by default

keith added 2 commits July 2, 2026 15:54
Previously since this defaulted to on, linking cgo with musl would fail
because cgo assumes pic is off by default
@keith

keith commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

the second commit here shows the issue with this approach. this breaks if you try to enable --force_pic unless you also set that for rules_go. this makes it pass:

diff --git a/tests/cgo/defs.bzl b/tests/cgo/defs.bzl
index 665010b..811a622 100644
--- a/tests/cgo/defs.bzl
+++ b/tests/cgo/defs.bzl
@@ -7,4 +7,7 @@ force_pic_go_binary, _force_pic_go_binary_internal = with_cfg(
 ).set(
     "force_pic",
     True,
+).set(
+    Label("@rules_go//go/config:linkmode"),
+    "pie",
 ).build()

we could maybe make rules_go read the cc* stuff for this, but I'm not sure if it's worth it. the nice thing about this change is that it works by default. we could also continue supporting //constraints/pie by making a new default of unset or something, and only respecting it when it was unset

@keith

keith commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

like this works to maintain override behavior:

diff --git a/constraints/pie/BUILD.bazel b/constraints/pie/BUILD.bazel
new file mode 100644
index 0000000..2542880
--- /dev/null
+++ b/constraints/pie/BUILD.bazel
@@ -0,0 +1,21 @@
+package(default_visibility = ["//visibility:public"])
+
+constraint_setting(
+    name = "pie",
+    default_constraint_value = "unset",
+)
+
+constraint_value(
+    name = "unset",
+    constraint_setting = "pie",
+)
+
+constraint_value(
+    name = "on",
+    constraint_setting = "pie",
+)
+
+constraint_value(
+    name = "off",
+    constraint_setting = "pie",
+)
diff --git a/toolchain/args/BUILD.bazel b/toolchain/args/BUILD.bazel
index c358fff..f05b12f 100644
--- a/toolchain/args/BUILD.bazel
+++ b/toolchain/args/BUILD.bazel
@@ -153,6 +153,20 @@ cc_args(
     },
 )
 
+cc_args(
+    name = "static_link_executable_custom_pic",
+    actions = [
+        "@rules_cc//cc/toolchains/actions:cpp_link_executable",
+        "@rules_cc//cc/toolchains/actions:lto_index_for_executable",
+        "@rules_cc//cc/toolchains/actions:lto_index_for_dynamic_library",
+        "@rules_cc//cc/toolchains/actions:lto_index_for_nodeps_dynamic_library",
+    ],
+    args = select({
+        "//constraints/pie:on": ["-static-pie"],
+        "//constraints/pie:off": ["-static"],
+    }),
+)
+
 cc_args(
     name = "static_link_executable_pic",
     actions = [
@@ -179,10 +193,13 @@ cc_args(
 
 cc_args_list(
     name = "static_link_executable",
-    args = [
-        ":static_link_executable_pic",
-        ":static_link_executable_no_pic",
-    ],
+    args = select({
+        "//constraints/pie:unset": [
+            ":static_link_executable_pic",
+            ":static_link_executable_no_pic",
+        ],
+        "//conditions:default": [":static_link_executable_custom_pic"],
+    }),
 )
 
 cc_args(

@keith

keith commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

rules_go stuff would be fixed by bazel-contrib/rules_go#4648

cc_args(
name = "static_link_executable_no_pic",
actions = [
"@rules_cc//cc/toolchains/actions:cpp_link_executable",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we factor out an action_set for these?

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

constraint_setting(
name = "pie",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we keep these around as no-op with deprecation warning for a bit? give rules_rs and anyone else using them a chance to upgrade?

@keith

keith commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

i will rebase once rules_go is released with that fix

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

Successfully merging this pull request may close these issues.

3 participants