-
Notifications
You must be signed in to change notification settings - Fork 90
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
Refactor Xcodeproject generation code. Add in xcode scheme test action env vars and command line args. #37
Conversation
@@ -1,7 +1,7 @@ | |||
load("@build_bazel_rules_apple//apple:providers.bzl", "AppleBundleInfo") | |||
load("@bazel_skylib//lib:paths.bzl", "paths") | |||
|
|||
def _get_transitive(deps, provider, field): | |||
def _get_attr_values_for_name(deps, provider, field): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT of this method rename? I thought this name was easier to understand, but open to hearing other options :)
@@ -81,11 +91,12 @@ def _xcodeproj_aspect_impl(target, ctx): | |||
actual = getattr(ctx.rule.attr, "tests")[0] | |||
elif ctx.rule.kind in ("alias"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On line 92, how does the in
keyword work. The line isctx.rule.kind in ("alias"):
Is ("alias")
treated as a list with one item?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a single item tuple
… ensure that the scheme for the xcodeproject has the correct env and args
@@ -81,11 +91,12 @@ def _xcodeproj_aspect_impl(target, ctx): | |||
actual = getattr(ctx.rule.attr, "tests")[0] | |||
elif ctx.rule.kind in ("alias"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a single item tuple
installer_path = ctx.executable.installer.short_path, | ||
) | ||
|
||
proj_options = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this refactor is great!!
@@ -234,6 +259,7 @@ xcodeproj = rule( | |||
"include_transitive_targets": attr.bool(default = False, mandatory = False), | |||
"project_name": attr.string(mandatory = False), | |||
"bazel_path": attr.string(mandatory = False, default = "bazel"), | |||
"scheme_existing_envvar_overrides": attr.string_dict(allow_empty = True, default = {}, mandatory = False), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a test project that uses this attr ?
tests/xcodeproj/build.sh
Outdated
@@ -2,4 +2,6 @@ set -eux | |||
|
|||
cd $(dirname $0) | |||
|
|||
xcodebuild -project Single-Application.xcodeproj -quiet | |||
xcodebuild -project Single-Application-Project.xcodeproj -quiet | |||
export SIM_DEVICE_ID=`xcodebuild -project Single-Application-Project.xcodeproj -scheme "Single-Application-UnitTests" -showdestinations | grep "platform:iOS Sim" | head -1 | ruby -e "puts STDIN.read.split(',')[1].split(':').last"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a set -o pipefail
since this file now uses pipes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will put back this change in later when I need it, right now it is not necessary run these tests. I think it will be necessary when the tests ensure that runs can run, when they are supposed to run inside an apphost.
@@ -0,0 +1,12 @@ | |||
import XCTest | |||
|
|||
class SwiftTests : XCTestCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like this test isn't passing when run directly via bazel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it looks like it invokes a test runner and complains the argument is invalid. So I had changed the argument to -h
, which is the help option for the test runner :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ha! it's possible command line args aren't yet possible to pass to https://github.com/google/xctestrunner, which is the underlying test runner the apple rules hook up
tests/xcodeproj/build.sh
Outdated
xcodebuild -project Single-Application.xcodeproj -quiet | ||
xcodebuild -project Single-Application-Project.xcodeproj -quiet | ||
export SIM_DEVICE_ID=`xcodebuild -project Single-Application-Project.xcodeproj -scheme "Single-Application-UnitTests" -showdestinations | grep "platform:iOS Sim" | head -1 | ruby -e "puts STDIN.read.split(',')[1].split(':').last"` | ||
xcodebuild -project Single-Application-Project.xcodeproj -scheme "Single-Application-UnitTests" test-without-building -destination "id=$SIM_DEVICE_ID" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might actually want test
here, since this scheme hasnt been built yet. also maybe ad -quiet
so the output is empty on success?
rules/xcodeproj.bzl
Outdated
arguments = ["--quiet", "--no-env", "--spec", xcodegen_yaml.path, "--project", project.dirname], | ||
inputs = depset([xcodegen_yaml], transitive = [target.srcs for target in targets]), | ||
arguments = ["--quiet", "--no-env", "--spec", xcodegen_jsonfile.path, "--project", project.dirname], | ||
inputs = depset([xcodegen_jsonfile], transitive = [target.srcs for target in targets]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wonder if we can stop providing the srcs as inputs to this action, since we manually mark all of the as validate: false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to this documentation, validate
is not a field, but optional
is. I have changed this to say optional: True
.
https://github.com/yonaskolb/XcodeGen/blob/master/Docs/ProjectSpec.md#sources
tests/xcodeproj/test.swift
Outdated
@@ -2,11 +2,7 @@ import XCTest | |||
|
|||
class SwiftTests : XCTestCase { | |||
func testPasses() { | |||
let envvarvalue = ProcessInfo.processInfo.environment["test_envvar_key"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you get rid of the tests for the args / env var?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment below. The functionality being added in this PR can be tested by doing git diff
on the xcodeproject file and ensuring it hasn't changed.
tests/xcodeproj/build.sh
Outdated
@@ -2,6 +2,4 @@ set -eux | |||
|
|||
cd $(dirname $0) | |||
|
|||
xcodebuild -project Single-Application-Project.xcodeproj -quiet | |||
export SIM_DEVICE_ID=`xcodebuild -project Single-Application-Project.xcodeproj -scheme "Single-Application-UnitTests" -showdestinations | grep "platform:iOS Sim" | head -1 | ruby -e "puts STDIN.read.split(',')[1].split(':').last"` | |||
xcodebuild -project Single-Application-Project.xcodeproj -scheme "Single-Application-UnitTests" test-without-building -destination "id=$SIM_DEVICE_ID" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not run the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we validate that the scheme in the xcodeproject is associated with the correct set of env vars and cli args (by running git diff on the project.pbxproj file), we already ensure that bazel has correctly associated schemes with the right set of env and args.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome!
No description provided.