Skip to content

Comments

cc_binary: support .dSYM generation#402

Merged
keith merged 1 commit intobazelbuild:masterfrom
benjivos:feat/support_dsym
Aug 1, 2025
Merged

cc_binary: support .dSYM generation#402
keith merged 1 commit intobazelbuild:masterfrom
benjivos:feat/support_dsym

Conversation

@benjivos
Copy link
Contributor

@benjivos benjivos commented Apr 17, 2025 •
edited
Loading

Add support for generating a .dSYM file if this is requested using --apple_generate_dsym. This change contains the following patches:

  • Add DSYM_HINT flags to be picked up by wrapped_clang to generate .dSYM bundles on request.
  • In wrapped_clang.cc make sure the linked binary is stripped after generating the .dSYM bundle first to make the .dSYM bundle contain useful information.

Adopts changes in bazel to support .dSYM files (as part of fixing bazelbuild/bazel#16893).

keith
keith reviewed Apr 17, 2025
View reviewed changes
Copy link
Member

@keith keith left a comment

Choose a reason for hiding this comment

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

Can you add a test similar to

apple_support/test/linking_tests.bzl

Lines 217 to 226 in af8d93a

dsym_test(
name = "{}_generate_dsym_test".format(name),
tags = [name],
expected_argv = [
"-g",
"DSYM_HINT_LINKED_BINARY",
],
mnemonic = "ObjcLink",
target_under_test = "//test/test_data:macos_binary",
)
for this? I think it will have to be disabled until your bazel change lands but at least that would be useful to verify locally too.

nit: please run pre-commit to format the code with clang-format

I think the test failure here is related to this change, not 100% sure

@benjivos benjivos force-pushed the feat/support_dsym branch from 2c041da to b800f37 Compare April 22, 2025 15:07
@benjivos benjivos requested a review from keith April 23, 2025 04:46
@benjivos benjivos mentioned this pull request Apr 23, 2025
Closed
@keith
Copy link
Member

keith commented Apr 24, 2025

i think this looks good but we need to wait for the bazel change to make sure this is the right path

@archshift
Copy link

archshift commented May 30, 2025

Any updates since the Bazel PR has been merged?

@benjivos
Copy link
Contributor Author

benjivos commented Jun 2, 2025

@keith should we wait until bazel 8.3 is released? (at least I could enable the test then)

@keith
Copy link
Member

keith commented Jul 5, 2025 •
edited
Loading

i think we can test this now, want to rebase?

@benjivos benjivos force-pushed the feat/support_dsym branch from b800f37 to 6673725 Compare July 15, 2025 06:56
@benjivos
Copy link
Contributor Author

benjivos commented Jul 15, 2025

@keith I rebased, but looks like it fails on bazel 7 build. How should I exclude the test from that configuration?

@keith
Copy link
Member

keith commented Jul 15, 2025

not right now but you can mess with

apple_support/.bazelci/presubmit.yml

Line 17 in 7009b77

- "//test/test_data:multi_arch_cc_binary"
to exclude this single target manually for 7.x only

@benjivos benjivos force-pushed the feat/support_dsym branch 4 times, most recently from 6f07490 to 914cf2b Compare July 17, 2025 07:34
@benjivos
Copy link
Contributor Author

benjivos commented Jul 17, 2025

@keith I was only able to manually add the new test target to the bazel 8+ tasks in .bazelci/presubmit.yml. I tried otherwise, but kept getting these failures when running with bazel 7:

ERROR: /Volumes/Projects/apple_support/test/BUILD:29:19: in dsym_test rule //test:linking_generate_cpp_dsym_test:
Traceback (most recent call last):
	File "/Volumes/Projects/apple_support/test/rules/action_command_line_test.bzl", line 80, column 40, in _action_command_line_test_impl
		concatenated_args = " ".join(action.argv) + " "
Error: Invalid toolchain configuration: Cannot find variable named 'dsym_path'.
ERROR: /Volumes/Projects/apple_support/test/BUILD:29:19: Analysis of target '//test:linking_generate_cpp_dsym_test' failed

@keith
Copy link
Member

keith commented Jul 29, 2025

hrm are we sure that doesn't actually break building cc_binarys with 7.x for real? that error makes it sound like if we were hitting the new codepath in the crosstool that it would fail because really you need expand_if_available for 2 attributes

@benjivos
cc_binary: support .dSYM generation
7c8e04f 
Add support for generating a .dSYM file if this is requested using `--apple_generate_dsym`. This change contains the following patches:

* Add `DSYM_HINT` flags to be picked up by `wrapped_clang` to generate .dSYM bundles on request.
* In `wrapped_clang.cc` make sure the linked binary is stripped after generating the .dSYM bundle first to make the .dSYM bundle contain useful information.

Adopts recent changes in bazel to support .dSYM files (as part of fixing bazelbuild/bazel#16893).
@benjivos benjivos force-pushed the feat/support_dsym branch from 914cf2b to 7c8e04f Compare August 1, 2025 17:47
@benjivos
Copy link
Contributor Author

benjivos commented Aug 1, 2025

hrm are we sure that doesn't actually break building cc_binarys with 7.x for real? that error makes it sound like if we were hitting the new codepath in the crosstool that it would fail because really you need expand_if_available for 2 attributes

I managed to remove the manual tag, and only exclude it as a test target for 7.x. I did this by only using expand_if_available for the dsym_path attribute, which seems to work. Let me know what you think.

@keith keith merged commit 9975f41 into bazelbuild:master Aug 1, 2025
11 checks passed
@vakhidbetrakhmadov vakhidbetrakhmadov mentioned this pull request Oct 10, 2025
Closed
@vakhidbetrakhmadov
Copy link

vakhidbetrakhmadov commented Oct 10, 2025 •
edited
Loading

Hello everyone.

It look like this change introduced a regression at least in test bundles, see #465.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@keith keith keith approved these changes

@aaronsky aaronsky Awaiting requested review from aaronsky

@BalestraPatrick BalestraPatrick Awaiting requested review from BalestraPatrick

@brentleyjones brentleyjones Awaiting requested review from brentleyjones

@luispadron luispadron Awaiting requested review from luispadron

@mattrobmattrob mattrobmattrob Awaiting requested review from mattrobmattrob

@thii thii Awaiting requested review from thii

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@benjivos @keith @archshift @vakhidbetrakhmadov