Force swift local debug option to be turned on so that framework search path has correct payload after project is built #101

gyfelton · 2020-08-14T20:21:55Z

Why this change

With the change introduced in #98, when bazel builds behind the scene (inside build-wrapper), it will generates one that enables swift to be debuggable. However the framework search paths generated during the xcodeproj generation stage (bazel run some_xcodeprojec_rule) will generate a different configuration cache than the one generated by bazel build (since inside build wrapper there are additional flags: 1. compilation mode and 2. local_debug_option)
As evidenced by the test fixture changes, the search paths are changed in this PR because

compilation_mode is set to debug to match with when xcode builds under debug config (not release config, so under release this is still broken)
local_debug_option is set to True for both xcode proj rule (through transition) and inside build-wrapper

The direct effect is that the framework search path is now pointing at the correct location.

Known issue:

Release config won't work with this change because compilation mode for debug is only set for Debug config, wondering if we should just have Release config pointing to compilation_mode = dbg too since the expectation is that ppl use release config under xcode is because they want to test the behaviors of the MACROS?

Tests

Existing fixtures
TODO: add a step after xcodebuild to verify that each framework specified under framework search path and header search path can be all found

segiddins · 2020-08-14T21:17:34Z

rules/transition_support.bzl

 transition_support = struct(
    apple_rule_transition = _apple_rule_transition,
    current_apple_platform = _current_apple_platform,
+    xcodeproj_rule_transition = _xcodeproj_rule_transition,


maybe it's a bit more clear to name this force_swift_local_debug_options_transition ?

segiddins · 2020-08-14T21:18:50Z

.bazelrc

@@ -17,3 +17,7 @@ build --deleted_packages tests/ios/frameworks/sources-with-prebuilt-binaries
 # when swiftmodule caching is enabled.
 # build --features=swift.cacheable_swiftmodules
 # build --features=swift.use_global_module_cache
+
+# Match this with the flag used in build-wrapper.sh
+build --compilation_mode=dbg


why is this necessary? builds in this rules repo should work in all 3 compilation modes

hmmm so the bazel-out directory name is a composition of compilation mode and configruation hash and other architecture parameters, right? If we don't specify this one, the framework search path will be applebin.....-fastbuild-<somehash> instead of applebin.....-debug-<somehash>. Am I correct here? (the change inside fixture also confirms this)

so unless we make the build-wrapper respect the compilation mode in first place instead of hardcoding?

either the build wrapper should respect the compilation mode or, my hunch is, we'll need a split transition so we can get the full matrix of settings for {debug,release}/{simulator,device}

will do the wrapper respect setting one and do the split transition to get full matrix of settings for {debug,release}/{simulator,device} in future

okay turns out adding the macro in the first place is not needed because if we specify compile mode to be dbg under .bzelrc, it will be respected inside the build-warpper. unless i missed some case previously discovered by @amberdixon

In its current form, this PR has removed compilation_mode=dbg from the build-wrapper. Could you put it back? You can add it into the new BAZEL_RULES_IOS_OPTIONS build setting you have created and are using.

we have discussed and agree that above comment is invalid as now the single point of truth is on user's .bazelrc file @amberdixon

segiddins · 2020-08-18T19:26:43Z

.github/workflows/tests.yml

          bazelisk query 'kind(xcodeproj, tests/macos/xcodeproj/...)' | xargs -n 1 bazelisk run
          bazelisk query 'attr(executable, 1, kind(genrule, tests/macos/xcodeproj/...))' | xargs -n 1 bazelisk run
          git diff --exit-code tests/macos/xcodeproj
+      - name: Ensure hmap files do not exist (or remove if they do)


why is this necessary? should we instead put this logic in ./tests/macos/xcodeproj/build.sh so it can be used when running locally?

new test case will fail if changes are not in :)

segiddins · 2020-08-18T19:27:38Z

tools/xcodeproj_shims/build-wrapper.sh

-# prefix to the `local_debug_options_enabled` build flag
-$BAZEL_PATH build --experimental_execution_log_file=$BAZEL_BUILD_EXECUTION_LOG_FILENAME --build_event_text_file=$BAZEL_BUILD_EVENT_TEXT_FILENAME --build_event_publish_all_actions $1 --@build_bazel_rules_ios//rules:local_debug_options_enabled $BAZEL_DEBUG_SYMBOLS_FLAG 2>&1 | $BAZEL_OUTPUT_PROCESSOR
+$BAZEL_PATH build \
+--experimental_execution_log_file=$BAZEL_BUILD_EXECUTION_LOG_FILENAME \


nit: indent subsequent lines so it's visually clear they're part of a single command invocation

amberdixon · 2020-08-18T20:27:39Z

.github/workflows/tests.yml

          bazelisk query 'kind(xcodeproj, tests/ios/xcodeproj/...)' | xargs -n 1 bazelisk run
          bazelisk query 'attr(executable, 1, kind(genrule, tests/ios/xcodeproj/...))' | xargs -n 1 bazelisk run
          git diff --exit-code tests/ios/xcodeproj
+      - name: Ensure hmap files do not exist (or remove if they do)
+        run: |
+          grep "HEADER_SEARCH_PATHS" tests/ios/xcodeproj/*.xcodeproj/project.pbxproj | grep -o  "bazel-out\S*\.hmap" | xargs -I {} bash -c 'F=$0; echo $F; if [ -f $F ]; then rm $F; echo "removed file at $F"; fi' "{}"


I think this was brought by Sam earlier ... but can you move these steps into the build.sh ? Thanks.

Also, I think these tests should all be referring to FRAMEWORK_SEARCH_PATHS, not HEADER_SEARCH_PATHS.

This is because in the lldb-settings.sh, we tell LLDB about the $FRAMEWORK_SEARCH_PATHS.

for now it does the same thing of checking against the hash value to see if what xcodeproj wants is what bazel build generates inside build-warapper.sh
This test case will fail if changes in this PR is reverted (the transitive rule) so i do see value in this test

and make build-wrapper more readable

…figuration hash

…c setting instead

…tions_transition `

…deproj is referring to do exist

gyfelton · 2020-08-18T22:25:28Z

@segiddins @amberdixon should be ready to review again 🙏

segiddins · 2020-08-18T23:45:33Z

.github/workflows/tests.yml

          bazelisk query 'kind(xcodeproj, tests/ios/xcodeproj/...)' | xargs -n 1 bazelisk run
          bazelisk query 'attr(executable, 1, kind(genrule, tests/ios/xcodeproj/...))' | xargs -n 1 bazelisk run
          git diff --exit-code tests/ios/xcodeproj
+      - name: Run pre build checks
+        run: ./tests/ios/xcodeproj/pre_build_check.sh


why not include these in the existing build shell script?

What's added are not part of build process (aka removing them won't affect build). might as well use a pre_build and post_build hooks for future iteration on such checks

gyfelton requested review from segiddins and amberdixon August 14, 2020 20:21

gyfelton changed the title ~~Fix framework search path not pointing to what bazel generates behind the scene~~ Fix framework search path not pointing to what bazel generates behind the scene Aug 14, 2020

segiddins reviewed Aug 14, 2020

View reviewed changes

gyfelton force-pushed the match_framework_search_path_to_actual_bazel_build branch from 4d99616 to aebd946 Compare August 17, 2020 19:32

gyfelton changed the title ~~Fix framework search path not pointing to what bazel generates behind the scene~~ Force swift local debug option to be turned on so that framework search path has correct payload after project is built Aug 17, 2020

segiddins reviewed Aug 18, 2020

View reviewed changes

amberdixon reviewed Aug 18, 2020

View reviewed changes

gyfelton added 9 commits August 18, 2020 18:06

Add new transition for xcodeproj

bf39fc8

Link xcodeproj rule to transition

f5df482

Use a macro to replace lcoal_debug flag inside build-wrapper

fc637fb

and make build-wrapper more readable

added debug as default compile mode to match with build-wrapper

e19e380

Update the fixtures to reflect the change on compilation mode and con…

0cfcc57

…figuration hash

fix buidifier

7188f09

remove hardcoded compilation mode macro and let it respect the bazelr…

e14f3d0

…c setting instead

rename the transition to be more generial `force_swift_local_debug_op…

788026c

…tions_transition `

Added pre and post build check scripts that will ensure the files xco…

88e4598

…deproj is referring to do exist

gyfelton force-pushed the match_framework_search_path_to_actual_bazel_build branch from 120955c to 88e4598 Compare August 18, 2020 22:06

indent the script so it makes more sense

31d0718

fixup

ec7a383

amberdixon approved these changes Aug 18, 2020

View reviewed changes

gyfelton merged commit 7457bd2 into master Aug 18, 2020

gyfelton deleted the match_framework_search_path_to_actual_bazel_build branch August 18, 2020 23:10

segiddins reviewed Aug 18, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force swift local debug option to be turned on so that framework search path has correct payload after project is built #101

Force swift local debug option to be turned on so that framework search path has correct payload after project is built #101

gyfelton commented Aug 14, 2020 •

edited

Loading

segiddins Aug 14, 2020

gyfelton Aug 18, 2020

segiddins Aug 14, 2020

gyfelton Aug 17, 2020

gyfelton Aug 17, 2020

segiddins Aug 17, 2020

gyfelton Aug 17, 2020

gyfelton Aug 17, 2020 •

edited

Loading

amberdixon Aug 18, 2020

gyfelton Aug 18, 2020

segiddins Aug 18, 2020

gyfelton Aug 18, 2020

segiddins Aug 18, 2020

amberdixon Aug 18, 2020

amberdixon Aug 18, 2020

gyfelton Aug 18, 2020

gyfelton commented Aug 18, 2020

segiddins Aug 18, 2020

gyfelton Aug 19, 2020

Force swift local debug option to be turned on so that framework search path has correct payload after project is built #101

Force swift local debug option to be turned on so that framework search path has correct payload after project is built #101

Conversation

gyfelton commented Aug 14, 2020 • edited Loading

Why this change

Known issue:

Tests

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

gyfelton Aug 17, 2020 • edited Loading

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

gyfelton commented Aug 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyfelton commented Aug 14, 2020 •

edited

Loading

gyfelton Aug 17, 2020 •

edited

Loading