Update bazel version to 3.0.0 and latest rules #45

gyfelton · 2020-04-16T16:07:57Z

Update bazel to 3.0.0 and fixes a bug where we pass mutable values (dict/list) to depset for env var and launch arguments for tests. Instead we pass tuples to ensure correct data is passed in a safer way.
Added tests on actual project side to ensure they will fail should the passing of the env var and launch arg not done correcetly.
Fixed test case failure due to latest bazel_swift rule update due to adding of modules maps from non-swift code, which is sth we don't anticipate.

segiddins

ERROR: /Users/runner/runners/2.169.0/work/rules_ios/rules_ios/tests/xcodeproj/BUILD.bazel:4:1: in //rules:xcodeproj.bzl%_xcodeproj_aspect aspect on _ios_internal_unit_test_bundle rule //tests/xcodeproj:Single-Application-UnitTests.__internal__.__test_bundle: 
Traceback (most recent call last):
	File "/Users/runner/runners/2.169.0/work/rules_ios/rules_ios/tests/xcodeproj/BUILD.bazel", line 4
		//rules:xcodeproj.bzl%_xcodeproj_aspect(...)
	File "/Users/runner/runners/2.169.0/work/rules_ios/rules_ios/rules/xcodeproj.bzl", line 58, in _xcodeproj_aspect_impl
		_TargetInfo(target = info, <1 more arguments>)
	File "/Users/runner/runners/2.169.0/work/rules_ios/rules_ios/rules/xcodeproj.bzl", line 58, in _TargetInfo
		depset([info], <1 more arguments>)

Looks like that will need to be changed to be immutable

rules/repositories.bzl

README.md

rules/repositories.bzl

gyfelton · 2020-04-17T22:23:13Z

@segiddins does it actually check if the generated xcodeproj works? I saw there is a check to see if bazel generates a different xcodeproj file, but it does not actuall test the project. I am at least adding some tests that will fail if I did not do the passing of test env correctly

segiddins · 2020-04-17T22:31:35Z

README.md

-
-[Click here](https://github.com/ob/rules_ios/tree/master/doc)
-for the documentation.
+Bazel version required by tThese rules is [here](https://github.com/ob/rules_ios/blob/master/.bazelversion)


README.md

-
-## Reference documentation
-
-[Click here](https://github.com/ob/rules_ios/tree/master/doc)


segiddins · 2020-04-17T22:32:32Z

rules/framework.bzl

@@ -155,6 +155,12 @@ def _apple_framework_packaging_impl(ctx):

            # collect modulemaps
            for modulemap in dep[apple_common.Objc].direct_module_maps:
+                # rule_swift changed how non swift generates module map in this commit


thanks for the comment here!

I'm confused about the original implementation here, as well as the change. If we assign modulemap_in to the modulemap on every iteration, aren't we just assigning modulemap_in with the last value in the array direct_module_maps ?

i have the same question actually! @segiddins

segiddins · 2020-04-17T22:33:01Z

tests/xcodeproj/Single-Application-Project.xcodeproj/project.pbxproj

@@ -17,6 +17,7 @@
 		48A8F4B41EF706F8BEEC7ACB /* Single-Application-UnitTests.xctest */ = {isa = PBXFileReference; includeInIndex = 0; lastKnownFileType = wrapper.cfbundle; path = "Single-Application-UnitTests.xctest"; sourceTree = BUILT_PRODUCTS_DIR; };
 		5FF81A9585DBB110EBD9CAC0 /* test.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; name = test.swift; path = test.swift; sourceTree = "<group>"; };
 		8DBB99CE021DEB0B97F196EA /* common.pch */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = common.pch; path = ../../rules/library/common.pch; sourceTree = "<group>"; };
+		BDFE1D6BB539968405F979CA /* Single_Application_UnitTests-Swift.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = "Single_Application_UnitTests-Swift.h"; path = "Single_Application_UnitTests-Swift.h"; sourceTree = "<group>"; };


cc @amberdixon on this getting added

is this expected?

I still don't know why this is getting added, but I'm seeing this in my PRs too. The underlying reason is that the Swift.h file is being treated as a source file (i.e. running File.is_source on it returns true), even though the file is actually being generated by one of the rules in rules_swift. Perhaps we can leave it here and make a note to remove it later, after figuring out why it's being added.

Do we have expectation that no file change should happen to project file?

yes, because there is a check in CI for git diff --nocheck ... so if the xcodeproj rule generates a project that is different from the one checked in already, then the CI job will fail.

I'm not sure why the -Swift.h file is showing up in the bazel target's sources as an actual source (because it's really a generated file.) Because it appears to be an actual source, the xcodeproj rule adds this file to the generated xcodeproject.

Will deal with this in future: #49

gyfelton · 2020-04-20T16:38:47Z

@segiddins While u continue to review, I am going to test this with our app repo and inform u on what works and what does not

segiddins

Looks good to me, other than the -Swift.h file getting added, curious what @amberdixon has to say about that

amberdixon · 2020-04-20T19:07:04Z

tests/xcodeproj/Single-Application-Project.xcodeproj/project.pbxproj

@@ -17,6 +17,7 @@
 		48A8F4B41EF706F8BEEC7ACB /* Single-Application-UnitTests.xctest */ = {isa = PBXFileReference; includeInIndex = 0; lastKnownFileType = wrapper.cfbundle; path = "Single-Application-UnitTests.xctest"; sourceTree = BUILT_PRODUCTS_DIR; };
 		5FF81A9585DBB110EBD9CAC0 /* test.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; name = test.swift; path = test.swift; sourceTree = "<group>"; };
 		8DBB99CE021DEB0B97F196EA /* common.pch */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = common.pch; path = ../../rules/library/common.pch; sourceTree = "<group>"; };
+		BDFE1D6BB539968405F979CA /* Single_Application_UnitTests-Swift.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = "Single_Application_UnitTests-Swift.h"; path = "Single_Application_UnitTests-Swift.h"; sourceTree = "<group>"; };


I still don't know why this is getting added, but I'm seeing this in my PRs too. The underlying reason is that the Swift.h file is being treated as a source file (i.e. running File.is_source on it returns true), even though the file is actually being generated by one of the rules in rules_swift. Perhaps we can leave it here and make a note to remove it later, after figuring out why it's being added.

amberdixon · 2020-04-20T19:07:40Z

tests/xcodeproj/test.swift

@@ -4,5 +4,12 @@ class SwiftTests : XCTestCase {
  func testPasses() {
    XCTAssertTrue(true)
  }
+  func testTestEnvArgsMatches() {
+    XCTAssertEqual(ProcessInfo.processInfo.environment["test_envvar_key1"], "test_envvar_value1")


This is fine to add here, but FYI, we aren't actually running these tests in CI.

I will probably run these tests as part of CI in a separate PR.

I am actually considering to try to add this into CI in this PR (otherwise my change has no effect)

amberdixon · 2020-04-20T19:12:42Z

rules/framework.bzl

@@ -155,6 +155,12 @@ def _apple_framework_packaging_impl(ctx):

            # collect modulemaps
            for modulemap in dep[apple_common.Objc].direct_module_maps:
+                # rule_swift changed how non swift generates module map in this commit


I'm confused about the original implementation here, as well as the change. If we assign modulemap_in to the modulemap on every iteration, aren't we just assigning modulemap_in with the last value in the array direct_module_maps ?

gyfelton · 2020-04-21T14:42:57Z

@amberdixon added the CI job in the workflow, can take a look?

amberdixon · 2020-04-21T21:27:46Z

tests/xcodeproj/tests.sh

+
+cd $(dirname $0)
+
+xcodebuild -project Single-Application-Project.xcodeproj -quiet -scheme Single-Application-UnitTests test


thanks for putting these in! LGTM

gyfelton · 2020-04-21T21:29:24Z

waiting for Sam's PR to merge so that I don't add the -Swift.h generated file

Update rules_apple and rules_swift to latest commit on their respective repo Added a stage to run unit tests inside the sample app application

segiddins reviewed Apr 16, 2020

View reviewed changes

rules/repositories.bzl Show resolved Hide resolved

README.md Show resolved Hide resolved

rules/repositories.bzl Outdated Show resolved Hide resolved

rules/repositories.bzl Show resolved Hide resolved

gyfelton force-pushed the elton/update_bazel_version branch from 6aa87c5 to 1fb34f8 Compare April 17, 2020 21:50

segiddins reviewed Apr 17, 2020

View reviewed changes

gyfelton mentioned this pull request Apr 17, 2020

Pass dependencies' module maps in SwiftInfo in a structured manner. bazelbuild/rules_swift#396

Merged

gyfelton changed the title ~~Update bazel version and rules~~ Update bazel version to 3.0.0 and latest rules Apr 17, 2020

gyfelton requested a review from amberdixon April 20, 2020 16:38

gyfelton assigned amberdixon and segiddins Apr 20, 2020

segiddins approved these changes Apr 20, 2020

View reviewed changes

amberdixon approved these changes Apr 20, 2020

View reviewed changes

gyfelton mentioned this pull request Apr 21, 2020

Header for swift is generated for unknown reason #49

Closed

gyfelton unassigned segiddins Apr 21, 2020

amberdixon reviewed Apr 21, 2020

View reviewed changes

gyfelton force-pushed the elton/update_bazel_version branch from ea7a3a5 to ced37b6 Compare April 21, 2020 22:11

Update bazel version to latest (3.0.0) and update readme.md

3c972d6

Update rules_apple and rules_swift to latest commit on their respective repo Added a stage to run unit tests inside the sample app application

gyfelton force-pushed the elton/update_bazel_version branch from ced37b6 to 3c972d6 Compare April 21, 2020 22:14

segiddins approved these changes Apr 22, 2020

View reviewed changes

justinseanmartin approved these changes Apr 22, 2020

View reviewed changes

gyfelton merged commit 73a4a1f into bazel-ios:master Apr 22, 2020

gyfelton deleted the elton/update_bazel_version branch April 22, 2020 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update bazel version to 3.0.0 and latest rules #45

Update bazel version to 3.0.0 and latest rules #45

gyfelton commented Apr 16, 2020 •

edited

Loading

segiddins left a comment

gyfelton commented Apr 17, 2020

segiddins Apr 17, 2020

This comment was marked as resolved.

This comment was marked as resolved.

segiddins Apr 17, 2020

amberdixon Apr 20, 2020

gyfelton Apr 20, 2020

segiddins Apr 17, 2020

gyfelton Apr 17, 2020

amberdixon Apr 20, 2020

gyfelton Apr 20, 2020

amberdixon Apr 20, 2020

gyfelton Apr 21, 2020

gyfelton commented Apr 20, 2020

segiddins left a comment

amberdixon Apr 20, 2020

amberdixon Apr 20, 2020

gyfelton Apr 20, 2020

amberdixon Apr 20, 2020

gyfelton commented Apr 21, 2020

amberdixon Apr 21, 2020

gyfelton commented Apr 21, 2020


		## Reference documentation

		[Click here](https://github.com/ob/rules_ios/tree/master/doc)


		cd $(dirname $0)

		xcodebuild -project Single-Application-Project.xcodeproj -quiet -scheme Single-Application-UnitTests test

Update bazel version to 3.0.0 and latest rules #45

Update bazel version to 3.0.0 and latest rules #45

Conversation

gyfelton commented Apr 16, 2020 • edited Loading

segiddins left a comment

Choose a reason for hiding this comment

gyfelton commented Apr 17, 2020

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyfelton commented Apr 20, 2020

segiddins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyfelton commented Apr 21, 2020

Choose a reason for hiding this comment

gyfelton commented Apr 21, 2020

gyfelton commented Apr 16, 2020 •

edited

Loading