[objective_c] Use user_defines to gate test utilities in build hook (Fixes #2999)#3325
Conversation
|
|
||
| hook: | ||
| user_defines: | ||
| include_test_utils: "false" |
| Objective-C. | ||
| - Fix a [bug](https://github.com/dart-lang/native/issues/3290) where missing | ||
| debug symbols caused app store valiadtion warnings. | ||
| - Moved test utilities to use `user_defines` in the build hook instead of a |
There was a problem hiding this comment.
From a user's perspective, they only care that some unnecessary stuff has been removed from the dylib. So how about "Removed some test-only utilities from the release dylib (fixes ..."
PR HealthChangelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with |
56498ad to
fefe07d
Compare
|
Addressed. Updated the default value and the changelog description. Thanks for the quick review @liamappelbe! |
| Objective-C. | ||
| - Fix a [bug](https://github.com/dart-lang/native/issues/3290) where missing | ||
| debug symbols caused app store valiadtion warnings. | ||
| - Removed some test-only utilities from the release dylib (fixes #2999). |
There was a problem hiding this comment.
nit: This sort of # shorthand doesn't work if the file isn't being viewed on github (eg on pub.dev). Use the same format as the other changelog entries.
fefe07d to
baa27bb
Compare
|
Fixed the changelog link format to use the full URL. Ready for another look, thanks! |
|
does this work when you run it locally? On CI I'm seeing errors that look like util.c is never included Eg: https://github.com/dart-lang/native/actions/runs/24811893252/job/72618483240?pr=3325. nsset_test.dart is a simple one to look at. |
baa27bb to
54a963f
Compare
|
Sorry about that @liamappelbe! The
I can't run the hook locally since I'm on Windows, but the fix should resolve the CI failures. Sorry for the noise! |
|
Looks pretty good. But you've got some formatting errors: https://github.com/dart-lang/native/actions/runs/24813053011/job/72801287014?pr=3325 |
54a963f to
3ce45cf
Compare
|
Fixed the formatting errors across the modified files. Ready for another look, thanks @liamappelbe :) |
|
Ok, looking closer at the errors, I think the issue is that util.dart is using the wrong |
3ce45cf to
da74ce7
Compare
|
Thanks @liamappelbe! Added |
|
Ok, the errors have changed a bit. Now they're complaining that the dylib doesn't contain the symbol. That's going to be hard for you to debug without a mac. I'll clone your PR and take a look. |
|
Thanks @liamappelbe, really appreciate you taking the time to clone and debug this directly. I'll wait for your findings! |
|
Thank you so much @liamappelbe for the fixes, the review, and for taking the time to debug this directly on your Mac. |
Description
Replaces the
objective_c_helperpackage approach from #3129 with a simpler solution usinguser_definesin the build hook, as suggested by @liamappelbe.The core idea:
user_definesdeclared inpubspec.yamlare only visible to the build hook when that package is the root/development package, not when it is a transitive dependency. This meanstest/util.cis only compiled into the dylib when actively developingobjective_c, keeping the production binary clean.Changes
testFilesconstant and the macOS-only gate fromhook/build.dart, replacing them with auser_defines-based gate (include_test_utils)hook.user_defines(include_test_utils: false) topubspec.yamlCHANGELOG.mdRelated Issues
Fixes #2999
Supersedes #3129
PR Checklist
test/util.cis no longer unconditionally compiled into the production dylib.CHANGELOG.mdfor theobjective_cpackage.