Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add force load libs to CcInfo linking context in Bazel 7 #811

Conversation

karim-alweheshy
Copy link
Contributor

@karim-alweheshy karim-alweheshy commented Dec 14, 2023

Issue

When we build an executable e.g.

bazelisk build //rules/test_host_app:iOS-14.0-AppHost

We get a linker error e.g.

ld: Undefined symbols:
  _main, referenced from:
      <initial-undefines>

Disgnostic

The linker args/flags file, e.g. iOS-14.0-AppHost_bin-2.params file, is missing the -force_load of the forced linked libs in the executable e.g.

-force_load
bazel-out/ios-sim_arm64-min14.0-applebin_ios-ios_sim_arm64-dbg-ST-261a193f71fb/bin/rules/test_host_app/libiOS-14.0-AppHost_objc.a

Resource

Following guide from here
Overall effort here
Example migrations for rules_apple here

Migration plan

  1. Move everything behind a bazel7 flag //rules:migrates_cc_info_linking_info
  2. A custom ObjC rule that generates ObjcProvider with linking info needs to generate a CcInfo with the same linking info.
  3. A custom ObjC rule that generates one of the providers used by avoid_deps needs to be modified to propagate an appropriate CcInfo.

@karim-alweheshy karim-alweheshy force-pushed the karim/fix.output_discriminator.usage-bazel7 branch from 78e3354 to 31e11cf Compare December 14, 2023 18:54
@karim-alweheshy karim-alweheshy mentioned this pull request Dec 20, 2023
7 tasks
@karim-alweheshy karim-alweheshy force-pushed the karim/fix.output_discriminator.usage-bazel7 branch 8 times, most recently from a42c44a to 7e5570a Compare December 20, 2023 18:59
@karim-alweheshy
Copy link
Contributor Author

After fixing the force_load issue we have another issue with

clang++: warning: using sysroot for 'iPhoneSimulator' but targeting 'MacOSX' [-Wincompatible-sysroot]

for all the linking on non MaxOSX targets

This will be addressed separately in another PR
This should just move us one step closer to bazel 7

@karim-alweheshy karim-alweheshy force-pushed the karim/fix.output_discriminator.usage-bazel7 branch 17 times, most recently from 8979d1d to c459bdf Compare December 21, 2023 14:44
@karim-alweheshy karim-alweheshy force-pushed the karim/fix.output_discriminator.usage-bazel7 branch 3 times, most recently from 8709099 to 34e7a7f Compare March 1, 2024 16:11
@karim-alweheshy karim-alweheshy force-pushed the karim/fix.output_discriminator.usage-bazel7 branch from ec7779c to ed7b03d Compare March 3, 2024 13:01
@karim-alweheshy karim-alweheshy force-pushed the karim/fix.output_discriminator.usage-bazel7 branch from ad4e0e9 to 04a2334 Compare March 4, 2024 11:31
@karim-alweheshy karim-alweheshy force-pushed the karim/fix.output_discriminator.usage-bazel7 branch from 04a2334 to 86a4f3d Compare March 4, 2024 13:04
luispadron added a commit that referenced this pull request Mar 5, 2024
luispadron added a commit that referenced this pull request Mar 6, 2024
@@ -121,7 +163,9 @@ def _framework_middleman(ctx):

framework_middleman = rule(
implementation = _framework_middleman,
attrs = dicts.add(split_transition_rule_attrs, {
fragments = ["cpp"],
toolchains = ["@bazel_tools//tools/cpp:toolchain_type"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you update this to use use_cpp_toolchain instead, similarly to what i did in #845

@@ -121,7 +163,9 @@ def _framework_middleman(ctx):

framework_middleman = rule(
implementation = _framework_middleman,
attrs = dicts.add(split_transition_rule_attrs, {
fragments = ["cpp"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is fragments here needed/do we need objc as well?

@@ -37,7 +37,7 @@ def _merge_objc_providers(providers, transitive = []):
)
return apple_common.new_objc_provider(**objc_provider_fields)

def _merge_dynamic_framework_providers(dynamic_framework_providers):
def _merge_dynamic_framework_providers(dynamic_framework_providers, supports_cc_info_in_dynamic_framework_provider):
Copy link
Collaborator

@luispadron luispadron Mar 6, 2024

Choose a reason for hiding this comment

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

nit: lets just check for bazel 7 here directly like we do elsewhere, it makes it easier to remove in the future than deleting the param

@@ -188,9 +237,13 @@ def _dep_middleman(ctx):
cc_providers = []
avoid_libraries = {}

_is_bazel_7 = not hasattr(apple_common, "apple_crosstool_transition")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mind updating all usage of this to use utils.bzl is_bazel_7, i landed that recently.

luispadron added a commit that referenced this pull request Mar 7, 2024
acecilia pushed a commit to revolut-mobile/rules_ios that referenced this pull request Apr 1, 2024
leonidmelnyk pushed a commit to leonidmelnyk/rules_ios that referenced this pull request Apr 4, 2024
tymurmustafaiev pushed a commit to tymurmustafaiev/rules_ios that referenced this pull request Apr 16, 2024
Rebased and fixed merge commit for bazel-ios#811

(cherry picked from commit fb395f9)
acecilia pushed a commit to revolut-mobile/rules_ios that referenced this pull request Apr 16, 2024
Rebased and fixed merge commit for bazel-ios#811

(cherry picked from commit fb395f9)
@luispadron
Copy link
Collaborator

Merged in #850

@luispadron luispadron closed this Apr 18, 2024
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