Remove unnecessary macOS crosstool configuration #13448

keith · 2021-05-08T00:57:52Z

This cleans up some unnecessary duplication / branching the macOS crosstool

These actions only differed on features that no-op in the same conditions so we don't need to duplicate these.

keith · 2021-05-08T00:58:38Z

tools/osx/crosstool/cc_toolchain_config.bzl

+            "unfiltered_compile_flags",
+            "compiler_input_flags",
+            "compiler_output_flags",
+            "unfiltered_cxx_flags",


This action only differed by having unfiltered_cxx_flags in 1 branch and not the other, this action exists for all archs, it's just a no-op for some, so we don't need to if on that in both places

keith · 2021-05-08T00:59:07Z

tools/osx/crosstool/cc_toolchain_config.bzl

+            flag_set(
+                flag_groups = [
+                    flag_group(flags = ["-stdlib=libc++", "-std=gnu++11"]),
+                    flag_group(flags = ["-arch", arch]),


This action only differed for different architectures by hardcoding them here, now that we have a variable for this we can remove that duplication

keith · 2021-05-08T00:59:42Z

tools/osx/crosstool/cc_toolchain_config.bzl

+            "version_min",
+            "apple_env",
+            "sysroot",
+            "cpp_linker_flags",


This action only differed by having cpp_linker_flags in 1 branch and not the other, this action exists for all archs, it's just a no-op for some, so we don't need to if on that in both places

keith · 2021-05-08T00:59:53Z

tools/osx/crosstool/cc_toolchain_config.bzl

+            "unfiltered_compile_flags",
+            "compiler_input_flags",
+            "compiler_output_flags",
+            "unfiltered_cxx_flags",


This action only differed by having unfiltered_cxx_flags in 1 branch and not the other, this action exists for all archs, it's just a no-op for some, so we don't need to if on that in both places

keith · 2021-05-08T01:00:07Z

tools/osx/crosstool/cc_toolchain_config.bzl

+            "unfiltered_compile_flags",
+            "compiler_input_flags",
+            "compiler_output_flags",
+            "unfiltered_cxx_flags",


This action only differed by having unfiltered_cxx_flags in 1 branch and not the other, this action exists for all archs, it's just a no-op for some, so we don't need to if on that in both places

This had 3 cases, the normal one that just differed by arch, the 3 macOS archs that just differed by arch, and 3 opt outs that don't pass `-target` (although maybe they should)

keith · 2021-05-08T01:09:32Z

tools/osx/crosstool/cc_toolchain_config.bzl

@@ -3314,187 +2712,9 @@ def _impl(ctx):
        requires = [feature_set(features = ["coverage"])],
    )

-    if (ctx.attr.cpu == "ios_arm64"):
-        default_link_flags_feature = feature(


default_link_flags has 3 cases, the macOS archs which just differed by arch, the normal case which just differed by arch, and the opt outs that don't pass -target which weren't changed here

This only differed by unfiltered_cxx_flags which exists for all configurations, but no-ops for some, so we don't have to duplicate that conditional

keith · 2021-05-08T01:12:24Z

tools/osx/crosstool/cc_toolchain_config.bzl

+            "unfiltered_compile_flags",
+            "compiler_input_flags",
+            "compiler_output_flags",
+            "unfiltered_cxx_flags",


This action only differed by unfiltered_cxx_flags which exists for all archs but just no-ops for some, so we don't have to duplicate that conditional

This only differed by unfiltered_cxx_flags which exists for all configurations, but no-ops for some, so we don't have to duplicate that conditional

keith · 2021-05-08T01:13:12Z

tools/osx/crosstool/cc_toolchain_config.bzl

+            "unfiltered_compile_flags",
+            "compiler_input_flags",
+            "compiler_output_flags",
+            "unfiltered_cxx_flags",


This action only differed by unfiltered_cxx_flags which exists for all archs but just no-ops for some, so we don't have to duplicate that conditional

keith · 2021-05-08T01:20:04Z

tools/osx/crosstool/cc_toolchain_config.bzl

+        ],
+    )
+
+    objcpp_compile_action = action_config(


This action only differed on arch, the apply_simulator_compiler_flags feature, which exists for all archs, just no-ops for some, so that's fine, and some actions used wrapped_clang instead of wrapped_clang_pp which seems to have been an unintentional difference

keith · 2021-05-08T01:22:14Z

tools/osx/crosstool/cc_toolchain_config.bzl

+                            "-filelist",
+                            "%{obj_list_path}",
+                            "-arch_only",
+                            target_system_name,


This action only differed on arch

keith · 2021-05-08T01:27:43Z

tools/osx/crosstool/cc_toolchain_config.bzl

+            ),
+            flag_set(
+                flag_groups = [
+                    flag_group(flags = ["-arch", target_system_name]),


this action only differed on arch

keith · 2021-05-08T01:29:00Z

tools/osx/crosstool/cc_toolchain_config.bzl

+            "version_min",
+            "apple_env",
+            "sysroot",
+            "cpp_linker_flags",


this action only differed on cpp_linker_flags which exists for all archs but no-ops for some

keith · 2021-05-08T01:29:59Z

tools/osx/crosstool/cc_toolchain_config.bzl

+            "version_min",
+            "apple_env",
+            "sysroot",
+            "cpp_linker_flags",


this action only differed on cpp_linker_flags which exists for all archs but no-ops for some

keith · 2021-05-08T01:31:06Z

tools/osx/crosstool/cc_toolchain_config.bzl

+            "unfiltered_compile_flags",
+            "compiler_input_flags",
+            "compiler_output_flags",
+            "unfiltered_cxx_flags",


this action only differed on unfiltered_cxx_flags which exists for all archs but no-ops for some

keith · 2021-05-08T01:33:35Z

tools/osx/crosstool/cc_toolchain_config.bzl

+                            "-no_warning_for_no_symbols",
+                            "-static",
+                            "-arch_only",
+                            target_system_name,


this action only differed on arch

keith · 2021-05-08T01:38:08Z

tools/osx/crosstool/cc_toolchain_config.bzl

-                                "-D__DATE__=\"redacted\"",
-                                "-D__TIMESTAMP__=\"redacted\"",
-                                "-D__TIME__=\"redacted\"",
+                                target_system_name,


this feature only differed on arch, except for armeabi-v7a which excludes the -target arg

keith · 2021-05-08T02:03:35Z

this diff is obviously quite large, happy to break it up however we want, right now reviewing by commit should be easiest, and I commented on every feature I changed. I aimed for 0 behavior changes here

trybka

I reviewed this to the best of my ability, and it seems reasonable.

Some notes for @keith :

Most of the remaining conditionals still seem unnecessarily verbose. In particular, can we remove armeabi-v7a? Also, the other pattern I see that could possibly be removed is like:
if (one condition) ... elif (other condition) ... else (default case which should probably never happen, i.e. CPU is not anything we recognize)

Maybe those could be collapsed a bit in a follow-up?
Certainly wouldn't block this on such a cleanup, though.

trybka · 2021-05-08T18:05:26Z

tools/osx/crosstool/cc_toolchain_config.bzl

+                    ACTION_NAMES.cpp_header_parsing,
+                    ACTION_NAMES.objc_compile,
+                    ACTION_NAMES.objcpp_compile,
+                    "objc-executable",


should these use ACTION_NAMES....executable?

hrm yea, there are quite a few of these that should switch to constants, mind if I do that in a follow up just to not add to the functional diff here?

sounds good

keith · 2021-05-08T19:14:58Z

Thanks for reviewing! If you're ok with me doing those in a followup I'd definitely prefer that at this point, and those should be much less churn than this diff. I've been trying to remember why this armeabi-v7a thing exists again but I'm happy to remove it in a follow up if it's not needed anymore

keith · 2021-05-08T20:45:48Z

Working on removing armeabi-v7a from this here #13449 but I would still prefer we land this first so I don't have to resolve conflicts between them, and in case I miss something there having to revert the whole thing would be tough

trybka · 2021-05-10T14:03:13Z

Follow-ups and conflict resolution all make sense to me. I'm happy to help land this as-is and then process the follow-up(s)

keith · 2021-05-10T14:06:15Z

Awesome thanks!

Remove unnecessary macOS crosstool configuration

fa8958c

These actions only differed on features that no-op in the same conditions so we don't need to duplicate these.

google-cla bot added the cla: yes label May 8, 2021

keith commented May 8, 2021

View reviewed changes

keith force-pushed the ks/remove-unnecessary-macos-crosstool-configuration branch from 754db4a to fa8958c Compare May 8, 2021 01:03

Update default_link_flags_feature

b5ccb79

This had 3 cases, the normal one that just differed by arch, the 3 macOS archs that just differed by arch, and 3 opt outs that don't pass `-target` (although maybe they should)

keith commented May 8, 2021

View reviewed changes

Deduplicate assemble_action

17b1a72

This only differed by unfiltered_cxx_flags which exists for all configurations, but no-ops for some, so we don't have to duplicate that conditional

keith commented May 8, 2021

View reviewed changes

Deduplicate preprocess_assemble_action

e8b80a3

This only differed by unfiltered_cxx_flags which exists for all configurations, but no-ops for some, so we don't have to duplicate that conditional

keith commented May 8, 2021

View reviewed changes

keith added 8 commits May 7, 2021 18:48

Dedup objcpp_compile_action

d8e5f7f

Dedup objc_archive_action

d7309f9

Dedup objc_executable_action

90f2711

Dedup cpp_link_executable_action

b6a03e6

Dedup cpp_link_nodeps_dynamic_library_action

5129d73

Dedup cpp_module_compile_action

aba4ac8

Dedup objc_fully_link_action

a8bd535

Dedup unfiltered_compile_flags_feature

63bb142

keith force-pushed the ks/remove-unnecessary-macos-crosstool-configuration branch from 4f9753e to 63bb142 Compare May 8, 2021 01:48

keith marked this pull request as ready for review May 8, 2021 02:15

trybka reviewed May 8, 2021

View reviewed changes

keith mentioned this pull request May 8, 2021

Remove armeabi-v7a from Apple crosstool #13449

Closed

keith mentioned this pull request May 11, 2021

Use -target instead of -arch in compiling commands in Apple crosstool #13466

Closed

bazel-io closed this in 0d7d969 May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unnecessary macOS crosstool configuration #13448

Remove unnecessary macOS crosstool configuration #13448

keith commented May 8, 2021

keith May 8, 2021

keith May 8, 2021

keith May 8, 2021

keith May 8, 2021

keith May 8, 2021

keith May 8, 2021

keith May 8, 2021

keith May 8, 2021

keith May 8, 2021

keith May 8, 2021

keith May 8, 2021

keith May 8, 2021

keith May 8, 2021

keith May 8, 2021

keith May 8, 2021

keith May 8, 2021

keith commented May 8, 2021

trybka left a comment

trybka May 8, 2021

keith May 8, 2021

trybka May 10, 2021

keith commented May 8, 2021

keith commented May 8, 2021

trybka commented May 10, 2021

keith commented May 10, 2021

Remove unnecessary macOS crosstool configuration #13448

Remove unnecessary macOS crosstool configuration #13448

Conversation

keith commented May 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith commented May 8, 2021

trybka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith commented May 8, 2021

keith commented May 8, 2021

trybka commented May 10, 2021

keith commented May 10, 2021