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 rules_apple 2.0.0-rc1 #269

Merged
merged 5 commits into from
Nov 11, 2022
Merged

Conversation

BalestraPatrick
Copy link
Member

We recently cut a new release of rules_apple that supports Bazel 6.0.0rc1, so this version should build in the BCR as well now.

@fmeum
Copy link
Contributor

fmeum commented Nov 8, 2022

@meteorcloudy The pipeline fails because bazelbuild/bazel#16578 is not in 6.0.0rc1. Assuming that we cherry-pick all Bzlmod changes to 6.0.0 anyway, should we run against last_green again?

@meteorcloudy
Copy link
Member

meteorcloudy commented Nov 8, 2022

Ah, thanks for the info! We also publish prebuilt Bazel binaries for commits in release-** branches. So we can just pin the Bazel version to the latest commit of release-6.0.0 (which is basically rc2 if we cut now)

@meteorcloudy meteorcloudy reopened this Nov 8, 2022
@meteorcloudy
Copy link
Member

@BalestraPatrick Can you push a dummy commit to re-trigger the presubmit? Rebuild doesn't apply my latest bazel version change.

@meteorcloudy
Copy link
Member

image

We are building with bazel@3dd01cbd44205c9157819ac77304354d08b8964a (ignore the 6.0.0 version label, fixing that in bazelbuild/continuous-integration#1491), but it's still failing..

@meteorcloudy
Copy link
Member

@BalestraPatrick Can you reproduce the same issue with last_green?

@BalestraPatrick
Copy link
Member Author

@meteorcloudy How does one reproduce these type of issues happening only in this BCR pipeline? I can build those targets just fine in rules_apple, but it's possible that some rules are depending on xctestrunner.

@meteorcloudy
Copy link
Member

I can build those targets just fine in rules_apple

Did you build rules_apple as the root module? It basically fails with a dummy module that depends on rules_apple like this
https://github.com/bazelbuild/continuous-integration/blob/master/buildkite/bazel-central-registry/bcr_presubmit.py#L162-L175

@meteorcloudy
Copy link
Member

I can reproduce locally:

pcloudy@pcloudy-macbookpro2:~/workspace/dummy_module
$ cat MODULE.bazel
bazel_dep(name = "rules_apple")
local_path_override(
    module_name = "rules_apple",
    path = "../rules_apple",
)
pcloudy@pcloudy-macbookpro2:~/workspace/dummy_module
$ bazel build --enable_bzlmod @rules_apple//examples/...
2022/11/09 10:50:59 WARN: Fallback to x86_64 because arm64 is not supported on Apple Silicon until 4.1.0
2022/11/09 10:50:59 Using unreleased version at commit 3e1b5fc4f9252211e6031dfa6cc424416988a51c
INFO: Invocation ID: 26c4ea5b-d956-47a2-ad77-d5e7447e58c8
INFO: Options provided by the client:
  Inherited 'common' options: --isatty=1 --terminal_columns=281
INFO: Reading rc options for 'build' from /Users/pcloudy/.bazelrc:
  'build' options: --verbose_failures --announce_rc --disk_cache=/tmp/bazel_disk_cache --repository_cache=/tmp/bazel_repository_cache --experimental_stats_summary --registry=file:///Users/pcloudy/workspace/bazel-central-registry
ERROR: /private/var/tmp/_bazel_pcloudy/7ecd4b046330144a8ede8cc7505e2ebd/external/rules_apple~override/apple/testing/default_runner/BUILD:73:16: every rule of type ios_test_runner implicitly depends upon the target '@[unknown repo 'xctestrunner' requested from @rules_apple~override]//:ios_test_runner', but this target could not be found because of: no such package '@[unknown repo 'xctestrunner' requested from @rules_apple~override]//': The repository '@[unknown repo 'xctestrunner' requested from @rules_apple~override]' could not be resolved: No repository visible as '@xctestrunner' from repository '@rules_apple~override'
Documentation for implicit attribute _testrunner of rules of type ios_test_runner:

It is the rule that needs to provide the AppleTestRunnerInfo provider. This
dependency is the test runner binary.
ERROR: Analysis of target '@rules_apple~override//examples/ios/PrenotCalculator:PrenotCalculatorTests' failed; build aborted:
INFO: Elapsed time: 0.108s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (1 packages loaded, 2 targets configured)

@BalestraPatrick
Copy link
Member Author

@meteorcloudy I solved those problems with the latest patch. It looks like the problem now is with the runfiles import not being found:

Traceback (most recent call last):
  File "/private/var/tmp/_bazel_buildkite/a03530b174ba10c2b7d83de4a046a70b/sandbox/darwin-sandbox/537/execroot/_main/bazel-out/darwin-fastbuild/bin/external/rules_apple~2.0.0-rc1/examples/multi_platform/Buttons/ButtonsLogicTests.runfiles/rules_apple~2.0.0-rc1~non_module_deps~xctestrunner/test_runner/ios_test_runner.py", line 28, in <module>
    from xctestrunner.shared import ios_constants
ModuleNotFoundError: No module named 'xctestrunner'

What's the expectation here? I see that bazelbuild/bazel#16124 (comment) (cc @fmeum) is basically complete, but I'm unsure if the expectation is for me to fix this upstream immediately? Or for this release I can even just exclude those 4 tests that are failing because of xctestrunner and move forward.

@meteorcloudy
Copy link
Member

meteorcloudy commented Nov 9, 2022

@BalestraPatrick Which runfiles library is rules_apple using? We still have some changes to cherry-pick to fix this problem.

I'm fine with temporarily excluding those tests in the first version of rules_apple.

@fmeum
Copy link
Contributor

fmeum commented Nov 9, 2022

@BalestraPatrick I had discussions with the Python rules owners and my takeaway was that Bazel users should move away from imports that mention repository names. Instead, all imports should rely on the fact that every repository root is implicitly added to the python path. If needed to resolve ambiguities, everything could be moved one level down into a top-level directory called (in this case) xctestrunner.

As a result bazelbuild/bazel#16124 will not fix this problem. Instead, you could patch xctestrunner to adopt this new structure and potentially submit the change upstream.

@fmeum
Copy link
Contributor

fmeum commented Nov 9, 2022

@BalestraPatrick I submitted google/xctestrunner#47. Could you try it out locally?

Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Thanks!

@meteorcloudy meteorcloudy merged commit 8ed3ebd into bazelbuild:main Nov 11, 2022
@BalestraPatrick
Copy link
Member Author

So I came up with two options to unblock this (while waiting for the above change to be merged in xctestrunner):

  • My preferred option would be to simply disable the tests that depend on xctestrunner, which I have done in this PR now. Hopefully it should turn green. This would cause users of our bzlmod integration to have broken tests, but I honestly don't expect anyone to use rules_apple with bzlmod since this is the first version we add and it's very much untested.
  • The second option is to somehow add xctestrunner at HEAD to the BCR with our patches on top of it. I prepared a branch here just in case: main...BalestraPatrick:bazel-central-registry:xctestrunner

On a separate note, it would be nice to add subpar to the BCR (since it's a dependency of xctestrunner and some other rulesets), but I just noticed that it's officially deprecated? https://github.com/google/subpar

@BalestraPatrick BalestraPatrick deleted the rules-apple branch November 11, 2022 13:55
@meteorcloudy
Copy link
Member

This would cause users of our bzlmod integration to have broken tests, but I honestly don't expect anyone to use rules_apple with bzlmod since this is the first version we add and it's very much untested.

I think it's totally fine for version 2.0.0-rc1 ;)

The second option is to somehow add xctestrunner at HEAD to the BCR with our patches on top of it. I prepared a branch here just in case: main...BalestraPatrick:bazel-central-registry:xctestrunner

Maybe it's better to introduce it via module extension? You can apply the same patch there.
Is xctestrunner like an implementation details of rules_apple. Or do you expect rules_apple users to specify their own xctestrunner version? If not, it probably shouldn't be in the BCR?

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