Skip to content

-sdk for Android (Take 3) #2633

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

Merged
merged 4 commits into from
Mar 3, 2020
Merged

Conversation

SDGGiesbrecht
Copy link
Contributor

This is a further narrowing of #2620. It now only changes anything when building for Android. It merely adds || triple.isAndroid().

The local variable of triple was necessary because || involves an implicit closure and would result in an error if the instance property were used directly:

'self' captured by a closure before all members were initialized

@SDGGiesbrecht SDGGiesbrecht requested a review from aciidgh as a code owner March 1, 2020 19:47
@aciidgh
Copy link
Contributor

aciidgh commented Mar 1, 2020

@SDGGiesbrecht LGTM but do you think it's possible to add a unit test for this? I want to avoid having these changes regress unintentionally.

@SDGGiesbrecht
Copy link
Contributor Author

Sure. Just a minute.

@SDGGiesbrecht
Copy link
Contributor Author

Done.

Destination’s synthesized initializer was unavailable from the test module, so I had to add a manual one.

@aciidgh
Copy link
Contributor

aciidgh commented Mar 1, 2020

Thanks!

@aciidgh
Copy link
Contributor

aciidgh commented Mar 1, 2020

@swift-ci smoke test

@SDGGiesbrecht
Copy link
Contributor Author

I see the failure. Looking into it.

@SDGGiesbrecht
Copy link
Contributor Author

On Linux it was attempting to validate the toolchain, while I was only giving it a mock value. I switched it to use the standard toolchain from Resources.default.

@aciidgh
Copy link
Contributor

aciidgh commented Mar 2, 2020

@swift-ci smoke test

@aciidgh
Copy link
Contributor

aciidgh commented Mar 2, 2020

@shahmishal can you give @SDGGiesbrecht commit/swift-ci trigger access?

@aciidgh
Copy link
Contributor

aciidgh commented Mar 2, 2020

@swift-ci smoke test linux

@aciidgh
Copy link
Contributor

aciidgh commented Mar 2, 2020

Oh the error seems related:

09:15:45 Test Case 'WorkspaceTests.testAndroidCompilerFlags' started at 2020-03-02 17:15:22.197
09:15:45 
<EXPR>:0: error: WorkspaceTests.testAndroidCompilerFlags : threw error "toolchain is invalid: Missing tool swiftc"
09:15:45 Test Case 'WorkspaceTests.testAndroidCompilerFlags' failed (0.022 seconds)
09:15:45 Test Suite 'WorkspaceTests' failed at 2020-03-02 17:15:22.219
0

@SDGGiesbrecht
Copy link
Contributor Author

SDGGiesbrecht commented Mar 2, 2020

Backing up the stack, the error is from:

  • Line 95 in UserToolchain.findProgram(_:envSearchPaths:), called from
  • Line 127 of UserToolchain.determineSwiftCompilers(binDir:lookup:envSearchPaths:), which is when there is no swiftc in the toolchain’s bin and there is no SWIFT_EXEC, called from
  • Line 238 of UserToolchain.init(destination:environment:) which is telling it to look in PATH.

So this test fails or not merely based on whether swiftc is available in PATH when the toolchain tries to validate itself. That makes it really complicated to reproduce locally.

I have narrowed the test by factoring the flag derivation out of the initializer and testing it in isolation. That way there is no UserToolchain instance trying to validate itself whatsoever. I think that should fix it, but then I never could reproduce the previous failures locally either.

@aciidgh
Copy link
Contributor

aciidgh commented Mar 3, 2020

@swift-ci smoke test

@SDGGiesbrecht
Copy link
Contributor Author

Finally, a green checkmark.

@aciidgh aciidgh merged commit b483bdd into swiftlang:master Mar 3, 2020
@aciidgh
Copy link
Contributor

aciidgh commented Mar 3, 2020

Thanks for the fix!

@SDGGiesbrecht SDGGiesbrecht deleted the sdk‐android branch March 3, 2020 20:23
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