Add apple_support 0.11.0 by BalestraPatrick · Pull Request #63 · bazelbuild/bazel-central-registry

BalestraPatrick · 2022-01-18T14:05:43Z

This is my first PR to this project and I'm still trying to get my head around all the tooling, so feedback is welcome.

This PR specifically adds apple_support for the release 0.11.0. I'm also about to open a PR to the same repo which adds a MODULE.bazel so that future releases won't need to have the same file checked-in into the registry.

meteorcloudy · 2022-01-18T14:33:00Z

I'm also about to open a PR to the same repo which adds a MODULE.bazel so that future releases won't need to have the same file checked-in into the registry.

To be clear, even if the MODULE.bazel is upstreamed, we still need to check it into the BCR, because this will allow us to do dependency resolution without downloading and unpacking the whole source archive.

keith · 2022-01-18T21:25:27Z

+    - '@apple_support//lib/...'
+
+bcr_test_module:
+  module_path: "test"


still surprised by this, and I assume this would break things in more complex repos wiht inter dependencies on absolute labels outside of the tests directory? 🤔

You are right, this isn't working correctly right now. Because the "test" directory needs to have a WORKSPACE file and work as a separate Bazel project which depends on the target module. I replied more here: bazelbuild/apple_support#100 (comment)

keith · 2022-01-18T21:25:52Z

+    run_test_module:
+      name: "Run test module"
+      platform: ${{ platform }}
+      build_targets:


should this not be test_targets or something? maybe one of the other identifiers is magic instead?

Good catch. I copy/pasted from protobuf, so I assume that's a bug?

Oh, the protobuf examples directory doesn't contain any tests, just build targets, which is also useful to verify if protobuf works correctly. That's why we have build_targets here.

I changed it to test_targets for us.

See bazelbuild/apple_support#100 (comment), without a WORKSPACE under test directory, you're actually running the tests from main module instead of a test module.

meteorcloudy · 2022-01-20T14:06:23Z

To fix the test failure, you'll have to also fix here: https://github.com/bazelbuild/apple_support/blob/master/lib/apple_support.bzl#L163

meteorcloudy · 2022-01-20T14:10:35Z

/cc @Wyverald
https://github.com/bazelbuild/apple_support/blob/a448644dce247e59268864fd66b6578195412b59/lib/apple_support.bzl#L163

IIUC, Label("//tools:xcode_path_wrapper") should correctly resolve to the apple support repo which contains the bzl file even if apple_support.action_required_attrs is used from a different repo, right?

Wyverald · 2022-01-20T15:13:36Z

Yes -- even without bzlmod, a repo-relative label there should work. In fact I believe it doesn't even need to be a Label(...) call; a bare string "//tools:xcode_path_wrapper" would work.

fmeum · 2022-01-20T15:27:33Z

Yes -- even without bzlmod, a repo-relative label there should work. In fact I believe it doesn't even need to be a Label(...) call; a bare string "//tools:xcode_path_wrapper" would work.

This would be very useful to know with certainty: Does attr.label automatically turn string arguments into labels based on its call site or does that only work when passed to rule's attrs? The latter definitely works.

Wyverald · 2022-01-20T16:13:08Z

Does attr.label automatically turn string arguments into labels based on its call site

Yes, so attr.label(default="X") and attr.label(default=Label("X")) are exactly equivalent, wherever they occur. (See source code in StarlarkAttrModule.java)

Label strings passed to the `default` argument of `attr.label()` are resolved to a `Label` immediately relative to the call site of `attr.label()`, so wrapping the argument with `Label(...)` is entirely redundant. More context at: bazelbuild/bazel-central-registry#63 (comment)

meteorcloudy · 2022-01-21T13:39:47Z

+index bd7c50b..33c9cbb 100644
+--- a/lib/apple_support.bzl
+++ b/lib/apple_support.bzl
+@@ -160,7 +160,7 @@ def _action_required_attrs():


Can you remove def _action_required_attrs():?

meteorcloudy

Looks great!

meteorcloudy · 2022-01-21T15:34:01Z

+ """Definition of a test rule to test apple_support."""
+
+ load(
+-    "@build_bazel_apple_support//lib:apple_support.bzl",


The whole patch file will be upstreamed, right?

Yes I will!

BalestraPatrick force-pushed the apple_support branch from 2b855f9 to d8b97c4 Compare January 18, 2022 14:26

BalestraPatrick force-pushed the apple_support branch from d8b97c4 to c545a90 Compare January 18, 2022 14:35

BalestraPatrick closed this Jan 18, 2022

BalestraPatrick deleted the apple_support branch January 18, 2022 15:04

BalestraPatrick restored the apple_support branch January 18, 2022 15:17

BalestraPatrick reopened this Jan 18, 2022

Add apple_support 0.11.0

79a7e6c

BalestraPatrick force-pushed the apple_support branch from c545a90 to 79a7e6c Compare January 18, 2022 15:29

meteorcloudy reviewed Jan 18, 2022

View reviewed changes

Comment thread modules/apple_support/0.11.0/presubmit.yml Outdated

meteorcloudy reviewed Jan 18, 2022

View reviewed changes

Comment thread modules/apple_support/metadata.json Outdated

Add all platforms and maintainer

2928651

keith reviewed Jan 18, 2022

View reviewed changes

Comment thread modules/apple_support/metadata.json

Comment thread modules/apple_support/0.11.0/MODULE.bazel

Comment thread modules/apple_support/0.11.0/presubmit.yml

Comment thread modules/apple_support/0.11.0/presubmit.yml

BalestraPatrick mentioned this pull request Jan 18, 2022

Add MODULE.bazel files bazelbuild/apple_support#100

Merged

BalestraPatrick force-pushed the apple_support branch from aaaa671 to c1e663d Compare January 18, 2022 20:08

BalestraPatrick added 2 commits January 18, 2022 21:14

Add patch to run apple_support tests

a651f53

Add keith as maintainer

3f484ba

BalestraPatrick force-pushed the apple_support branch from dc3ef7d to 3f484ba Compare January 18, 2022 20:14

Run tests only on macOS

51e7233

keith reviewed Jan 18, 2022

View reviewed changes

BalestraPatrick added 3 commits January 18, 2022 23:08

Change build_targets to test_targets

e253643

Patch test labels not to reference the main module

f6f470c

Remove bcr_test_module

310eeb8

meteorcloudy reviewed Jan 20, 2022

View reviewed changes

Comment thread modules/apple_support/0.11.0/presubmit.yml Outdated

BalestraPatrick force-pushed the apple_support branch from d7896aa to 558cf01 Compare January 20, 2022 13:58

meteorcloudy reviewed Jan 20, 2022

View reviewed changes

Comment thread modules/apple_support/0.11.0/presubmit.yml Outdated

Fix syntax

509edf7

BalestraPatrick force-pushed the apple_support branch from 558cf01 to 509edf7 Compare January 20, 2022 14:03

fmeum mentioned this pull request Jan 20, 2022

Do not mention redundant use case for Label constructor bazelbuild/bazel#14609

Closed

Add another label patch

edf5cff

BalestraPatrick force-pushed the apple_support branch from 377b1ae to edf5cff Compare January 20, 2022 17:12

meteorcloudy reviewed Jan 21, 2022

View reviewed changes

BalestraPatrick force-pushed the apple_support branch from aba3e62 to d21a8bb Compare January 21, 2022 13:58

Update patch

fbab00b

BalestraPatrick force-pushed the apple_support branch from d21a8bb to fbab00b Compare January 21, 2022 14:41

Add platforms dependency

74ca0af

meteorcloudy approved these changes Jan 21, 2022

View reviewed changes

meteorcloudy merged commit 455e4c6 into bazelbuild:main Jan 21, 2022

BalestraPatrick deleted the apple_support branch January 21, 2022 15:44

Conversation

BalestraPatrick commented Jan 18, 2022

Uh oh!

meteorcloudy commented Jan 18, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

meteorcloudy commented Jan 20, 2022

Uh oh!

meteorcloudy commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Wyverald commented Jan 20, 2022

Uh oh!

fmeum commented Jan 20, 2022

Uh oh!

Wyverald commented Jan 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meteorcloudy left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

meteorcloudy commented Jan 20, 2022 •

edited

Loading