Support rules_apple 3.x.x #773

jerrymarino · 2023-09-29T22:49:16Z

update rules_ios to use rules_apple 3.x.x
Adding support for rules_apple version 3. I have added a simple starlark split transition to pass existing tests that failed in the original update.
Add explict rules_apple API wrapper. Eventually all uses and hacks to make rules_apple work can flow through here.
Add CI job for rules_apple 2 - ( default is now 3 )

thiagohmcruz

I paused mid review as I wanted to clarify one thing first. What is the problem we're solving by adding all this additional handling for old versions? What is preventing us from simply moving on to Bazel 6 + rules_apple 3.x at HEAD?

I assume the intent is to support older versions for a longer period of time in case someone needs it. Why not do what other rule sets are doing then and (1) define the latest versions as "minimum" or "current" and (2) agree on a process to update older releases of rules_ios with critical bug fixes if necessary?

Not against the backwards-compatibility changes but wanted to understand if we absolutely need it to be implemented this way or not? Have you considered alternatives like mentioned above?

Maintaining two Bazel versions and two rules_apple versions at HEAD will increase maintenance cost for all of us and from a readability perspective there's def some overhead being added here for maintainers to have to understand what the split is doing exactly and why they can't just use known provider names in favor of the new_{SomeProvider} pattern.

cc @jszumski @luispadron you have more context than us on this. Does the above makes sense? Are we missing alternative approaches here and have we considered pros & cons of each?

.github/workflows/tests.yml

rules/framework.bzl

rules/repositories.bzl

rules/test.bzl

jerrymarino · 2023-10-17T01:35:28Z

@thiagohmcruz I've pushed the latest updates based on what we discussed offline. For now, we've determined to continue to support the older versions on HEAD and will have to re-evaluate at a later day

luispadron

Some comments, I still don't understand why we gate the rules_apple version based on the Bazel version.

luispadron · 2023-10-17T02:28:02Z

.github/workflows/tests.yml

          path: bazel-testlogs

-  lts_ios_integration_tests:
+  bazel_lts_ios_integration_tests:


IMO the name here is confusing, if it's going to change in this PR could we add bazel5 to the name instead of LTS.

LTS applies to multiple bazel versions including 6

I'm open to suggestions - what do you think a better name for it would be?

i think adding bazel5 like i said would make it clear this specifically is a bazel5 job.

+1 on being more explicit on the naming here

Ok sounds good to me - this is a good improvement in the github UI

luispadron · 2023-10-17T02:28:52Z

.github/workflows/tests.yml

      - uses: actions/checkout@v3
      - name: Preflight Env
-        run: .github/workflows/preflight_env.sh
+        run: .github/workflows/preflight_env.sh --no-bzlmod


Bzlmod support was merged with 6.1.2, why should this be disabled here?

Please find in in the proceeding comment that it uses 6.1.2 and rules_apple 2 - where bzlmod is hardcoded to use 3.0.0.

Gotcha so it doesnt work yet with rules_apple 3. I guess I got confused because we're essentially using Bazel 6.1.2 to mean rules_apple 2.

Not for this PR, but in the future I wonder if specifying these Bazel version + rules_apple versions wouldn't be better in a specific WORKSPACE for that specific test. Some repos do this with their integration tests and it allows them to test the different bazel version / rule versions they support without setting one version/feature for all their tests

Nit: I'd still explicitly say in that comment why that implies --no-bzlmod is needed. The same way it's confusing during review it will be once we're changing this in the future.

Saw what @luispadron is talking about in other repos too and also like the idea. Wondering if we should create "Feature Request" templates for issues in rules_ios to track things like this 🤔

Yeah it basically exists because people aren't using bzlmod on 5 and 6 so it's good to have some coverage of both ways to test this. Though, we could bump to a later version but needs more consideration.

Also, the way it's configured now get's us a larger matrix of bzlmod vs non bzlmod across a few versions. Though bzlmod support works on all the versions in the repo 5.4.1, 6.x.x we'd still like like the coverage to we don't break it - but probably not testing 100% of permutations

luispadron

Can we keep using the release versions, same for rules_swift.

rules/repositories.bzl

luispadron

In general, I'm confused why rules_apple version 2 is used in any of these checks. Don't we just need to check is this rules_apple 3? and use the appropriate provider methods?

rules/repositories.bzl

jerrymarino · 2023-10-17T17:14:11Z

In general, I'm confused why rules_apple version 2 is used in any of these checks.

We agreed to support 2.x.x on HEAD offline as a team as it's what we use in some key use case

luispadron · 2023-10-17T17:37:06Z

In general, I'm confused why rules_apple version 2 is used in any of these checks.

We agreed to support 2.x.x on HEAD offline as a team as it's what we use in some key use case

But now we seem to have no ios integration tests running Bazel 6 with rules_apple 3 & bzlmod or am I missing it in there. Just want to make sure we still test everything with the "latest" i.e. Bazel 6.3.2 and rules_apple 3 and bzlmod I see its here now:

rules_ios/.github/workflows/tests.yml

Line 15 in 9b0fe3f

integration_tests:

👍🏼

I wasn't part of that specific discussion but my understanding is ya'll agreed on not dropping Bazel 5 tests from the repository. This seems to add another CI requirement that rules_apple 2 is also supported with this specific Bazel 6.1.x configuration.

jerrymarino · 2023-10-17T22:23:00Z

This seems to add another CI requirement that rules_apple 2 is also supported with this specific Bazel 6.1.x configuration.

Yes, it does have to to support rules_apple 2 is because we will use this version and have to support it for us. We can move the bazel version forward from 6.1.2 but will require more followups to the testing infra and it doesn't impact anything beyond our testing infra.

1. update rules_ios to use rules_apple 3.x.x 2. Adding support for rules_apple version 3. I have added a simple starlark split transition to pass existing tests that failed in the original update. 3. Add explict rules_apple API wrapper. Eventually all uses and hacks to make rules_apple work can flow through here. 4. Add CI job for rules_apple 2 - ( default is now 3 )

…nternal targets.

jszumski · 2023-10-19T14:56:39Z

I'm not sure this was fully baked, I still see a bunch of linker warnings in the log:

INFO: From Linking tests/ios/frameworks/mixed-source/only-source/MixedSourceTest.__internal__.__test_bundle_bin:
ld: warning: ld: warning: object file (bazel-out/ios-x86_64-applebin_ios-ios_x86_64-dbg-ST-78e2e727b17e/bin/tests/ios/frameworks/mixed-source/only-source/libMixedSourceTestLib_swift.a(Tests.swift.o)) was built for newer iOS Simulator version (16.2) than being linked (12.0)
object file (bazel-out/ios-x86_64-applebin_ios-ios_x86_64-dbg-ST-78e2e727b17e/bin/tests/ios/frameworks/mixed-source/only-source/libMixedSourceTestLib_objc.a(ObjCTest.o)) was built for newer iOS Simulator version (16.2) than being linked (12.0)
ld: warning: object file (bazel-out/ios-x86_64-applebin_ios-ios_x86_64-dbg-ST-78e2e727b17e/bin/tests/ios/frameworks/mixed-source/only-source/libMixedSourceFramework_objc.a(AngleBracketNamespacedLogger.o)) was built for newer iOS Simulator version (16.2) than being linked (12.0)

jerrymarino · 2023-10-19T18:36:43Z

@jszumski thanks for raising it!

It's possible that we might need to tackle some followups or other improvements to it in general - I'm happy to review a PR to tackle it. We did this in a way that you can still use rules_apple 2.x.x and may have smaller incremental improvements to 3.x.x. now that this landed 🚀

jerrymarino · 2023-10-19T18:41:25Z

Also perhaps starlark validation of versions may be useful here outside of linker errors in general

jszumski · 2023-10-19T18:55:26Z

Somewhat false alarm, that warning has been present in master for a long time (as far back as our GitHub Action log retention, at least!). I'll add a todo to bisect the original cause

jerrymarino requested review from jszumski and thiagohmcruz September 29, 2023 22:49

jerrymarino force-pushed the jmarino/rules_apple_3_x branch 4 times, most recently from 4081e23 to 8558602 Compare September 30, 2023 00:41

luispadron mentioned this pull request Oct 3, 2023

Add support for bzlmod #703

Merged

thiagohmcruz reviewed Oct 3, 2023

View reviewed changes

.github/workflows/tests.yml Show resolved Hide resolved

.github/workflows/tests.yml Outdated Show resolved Hide resolved

rules/framework.bzl Show resolved Hide resolved

rules/framework.bzl Show resolved Hide resolved

jszumski reviewed Oct 3, 2023

View reviewed changes

rules/repositories.bzl Outdated Show resolved Hide resolved

rules/test.bzl Show resolved Hide resolved

jerrymarino force-pushed the jmarino/rules_apple_3_x branch from c32f697 to 69e63f0 Compare October 17, 2023 00:56

jerrymarino enabled auto-merge (squash) October 17, 2023 01:00

jerrymarino force-pushed the jmarino/rules_apple_3_x branch from 69e63f0 to 9b0fe3f Compare October 17, 2023 01:19

luispadron reviewed Oct 17, 2023

View reviewed changes

This comment was marked as duplicate.

Sign in to view

luispadron reviewed Oct 17, 2023

View reviewed changes

rules/repositories.bzl Show resolved Hide resolved

luispadron reviewed Oct 17, 2023

View reviewed changes

rules/repositories.bzl Show resolved Hide resolved

jszumski mentioned this pull request Oct 17, 2023

precompiled_apple_resource_bundle still requires common_tool_attributes that's not exist in rules_apple newest version #779

Closed

jerrymarino force-pushed the jmarino/rules_apple_3_x branch from 9b0fe3f to dcc51f6 Compare October 17, 2023 19:01

jerrymarino force-pushed the jmarino/rules_apple_3_x branch from dcc51f6 to ced7520 Compare October 17, 2023 23:55

thiagohmcruz approved these changes Oct 18, 2023

View reviewed changes

jerrymarino force-pushed the jmarino/rules_apple_3_x branch from c875233 to df24c71 Compare October 18, 2023 23:48

jerrymarino and others added 3 commits October 18, 2023 16:57

Bump projects

5c4f8eb

Fix linker warning when minimum_os_version is not set on a test's i…

6b53b4b

…nternal targets.

jerrymarino force-pushed the jmarino/rules_apple_3_x branch from df24c71 to 71f5031 Compare October 19, 2023 01:19

jerrymarino disabled auto-merge October 19, 2023 01:20

jerrymarino enabled auto-merge (squash) October 19, 2023 01:20

jerrymarino disabled auto-merge October 19, 2023 01:20

jerrymarino enabled auto-merge (squash) October 19, 2023 01:20

jerrymarino force-pushed the jmarino/rules_apple_3_x branch from 71f5031 to 2abd5bc Compare October 19, 2023 01:40

Adding for bzlmod

7efdb03

jerrymarino force-pushed the jmarino/rules_apple_3_x branch from 2abd5bc to 7efdb03 Compare October 19, 2023 01:58

jerrymarino merged commit 6f5c50e into master Oct 19, 2023

jerrymarino deleted the jmarino/rules_apple_3_x branch October 19, 2023 02:27

jszumski mentioned this pull request Oct 20, 2023

Update dependencies and use latest archives #784

Merged

thiagohmcruz mentioned this pull request Oct 26, 2023

Allow Bazel 6 users to explicitly opt-in to rules_apple 2.x #793

Merged

luispadron mentioned this pull request Oct 30, 2023

Feature Request - Migrate to CcInfo instead of ObjcProvider for Objective-C linking #757

Closed

luispadron mentioned this pull request Dec 15, 2023

Fix usage of apple_api_version #814

Merged

Support rules_apple 3.x.x #773

Support rules_apple 3.x.x #773

Uh oh!

Conversation

jerrymarino commented Sep 29, 2023

Uh oh!

thiagohmcruz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jerrymarino commented Oct 17, 2023

Uh oh!

luispadron left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luispadron Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luispadron left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as duplicate.

Uh oh!

Uh oh!

luispadron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jerrymarino commented Oct 17, 2023

Uh oh!

luispadron commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jerrymarino commented Oct 17, 2023

Uh oh!

jszumski commented Oct 19, 2023

Uh oh!

jerrymarino commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jerrymarino commented Oct 19, 2023

Uh oh!

jszumski commented Oct 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thiagohmcruz left a comment •

edited

Loading

luispadron left a comment •

edited

Loading

luispadron Oct 17, 2023 •

edited

Loading

luispadron commented Oct 17, 2023 •

edited

Loading

jerrymarino commented Oct 19, 2023 •

edited

Loading