Skip to content

Conversation

@dierksen
Copy link
Contributor

Tested locally to ensure that testonly = True is added only to the appropriate targets.

Copy link
Contributor

@thiagohmcruz thiagohmcruz left a comment

Choose a reason for hiding this comment

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

Aren't there any tests for this? If not maybe we can add one?

Copy link
Collaborator

@luispadron luispadron left a comment

Choose a reason for hiding this comment

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

Nice we're doing this in our rules currently but this makes more sense.

@dierksen dierksen merged commit d84dafd into master Nov 18, 2022
@dierksen dierksen deleted the dierksen/testonly branch November 18, 2022 17:23
@dierksen dierksen restored the dierksen/testonly branch November 18, 2022 17:42
@dierksen dierksen deleted the dierksen/testonly branch November 18, 2022 17:42
@jerrymarino
Copy link
Contributor

@justinseanmartin I wonder if can add thse flags differently based on what Maxwell added to rules_swift to get it working with pods

What do you think if we added back the old behavior into rules_ios conditionally - or adding more intricate logic to detect adding it? I'm still not 100% sure the original motivation to gut the flags.

@jerrymarino
Copy link
Contributor

@luispadron
Copy link
Collaborator

We'll need to update rules_ios since this now passes testonly for unit test targets which causes duplicate args because rules_ios always passes testonly = True for all test targets as well.


00:05:44.389 	File "/private/var/tmp/_bazel_build/5f8e42350ee950cd952d5c32bb6b1f7e/external/build_bazel_rules_ios/rules/test.bzl", line 61, column 28, in _ios_test
00:05:44.389 		library = apple_library(name = name, namespace_is_module_name = False, platforms = {"ios": ios_test_kwargs.get("minimum_os_version")}, testonly = True, **kwargs)
00:05:44.389 	File "/Users/build/.jenkins/workspace/PR+cas+BCVAFG+PR-Bazel-Optional/ios-builder/s/c/Tools/Rules/library.bzl", line 14, column 5

@luispadron
Copy link
Collaborator

This should fix the above issue: bazel-ios/rules_ios#609

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.

7 participants