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

Consider `XCODE_PRODUCT_BUILD_VERSION` alongside `EXPANDED_CODE_SIGN_IDENTITY` to address Xcode 10. #2476

Merged
merged 1 commit into from Jun 26, 2018

Conversation

Projects
None yet
4 participants
@jdhealy
Member

jdhealy commented Jun 6, 2018

Fixes #2472.

Treat EXPANDED_CODE_SIGN_IDENTITY — when EXPANDED_CODE_SIGN_IDENTITY is unset and XCODE_PRODUCT_BUILD_VERSION is set — with the same behavior as having empty string.

Xcode beta release 10L176w (and potentially more) seem to omit environment variables set to an empty string in Run Script Phases (and potentially other places). So with the heuristic of XCODE_PRODUCT_BUILD_VERSION being set signifying a Run Script Phases (or other within Xcode scenario), ignore the error by returning nil, as is done when codeSigningAllowed() is false.

Credit and thanks to @clayellis for reporting the behavior change and iterating on code changes.

Treat `EXPANDED_CODE_SIGN_IDENTITY` — when `EXPANDED_CODE_SIGN_IDENTI…
…TY` is unset and `XCODE_PRODUCT_BUILD_VERSION` is set — with the same behavior as having empty string.

Fixes <#2472>.

Xcode beta release `10L176w` (and potentially more) seem to omit environment variables set to an empty string in Run Script Phases (and potentially other places). So with the heuristic of `XCODE_PRODUCT_BUILD_VERSION` being set signifying a Run Script Phases (or other within Xcode scenario), ignore the error by returning `nil`, as is done when `codeSigningAllowed()` is false.

Credit and thanks to @clayellis for reporting the behavior change and iterating on code changes.
@jdhealy

This comment has been minimized.

Show comment
Hide comment
@jdhealy

jdhealy Jun 6, 2018

Member

@clayellis I gave you push access to the jdhealy/Carthage fork — if there’s any comments or code changes you’d like to push to this PR.

Member

jdhealy commented Jun 6, 2018

@clayellis I gave you push access to the jdhealy/Carthage fork — if there’s any comments or code changes you’d like to push to this PR.

@jdhealy jdhealy requested a review from blender Jun 6, 2018

@blender

This comment has been minimized.

Show comment
Hide comment
@blender

blender Jun 7, 2018

Member

XCODE_PRODUCT_BUILD_VERSION is only set on Xcode >= 10 ?

Member

blender commented Jun 7, 2018

XCODE_PRODUCT_BUILD_VERSION is only set on Xcode >= 10 ?

@jdhealy

This comment has been minimized.

Show comment
Hide comment
@jdhealy

jdhealy Jun 7, 2018

Member

XCODE_PRODUCT_BUILD_VERSION isn’t anything new, it gets included in much older Xcodes.

The thought is: all Xcode ‘Run Script Phases’ get the same behavior, but outside of Xcode (via the heuristic of XCODE_PRODUCT_BUILD_VERSION) — where there’s actually utility in guiding the user through what’s necessary to add to achieve (for what most is the goal) ensuring that code signing happens — we surface the error of unset EXPANDED_CODE_SIGN_IDENTITY.

Should that be made more clear in the source comments?

Member

jdhealy commented Jun 7, 2018

XCODE_PRODUCT_BUILD_VERSION isn’t anything new, it gets included in much older Xcodes.

The thought is: all Xcode ‘Run Script Phases’ get the same behavior, but outside of Xcode (via the heuristic of XCODE_PRODUCT_BUILD_VERSION) — where there’s actually utility in guiding the user through what’s necessary to add to achieve (for what most is the goal) ensuring that code signing happens — we surface the error of unset EXPANDED_CODE_SIGN_IDENTITY.

Should that be made more clear in the source comments?

// … or potentially non-beta Xcode releases of major version 10 or later.
switch getEnvironmentVariable("XCODE_PRODUCT_BUILD_VERSION") {
case .success(_):

This comment has been minimized.

@kballard

kballard Jun 8, 2018

Contributor

BTW, I don't know if this is some particular Carthage style you're following here, but in general you can omit the (_) and just say case .success: if you don't want any of the associated values.

@kballard

kballard Jun 8, 2018

Contributor

BTW, I don't know if this is some particular Carthage style you're following here, but in general you can omit the (_) and just say case .success: if you don't want any of the associated values.

@blender

looks good to me

@mdiep

mdiep approved these changes Jun 13, 2018

@mdiep mdiep merged commit cf009b7 into Carthage:master Jun 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jdhealy

This comment has been minimized.

Show comment
Hide comment
@jdhealy

jdhealy Sep 9, 2018

Member

Xcode beta release 10L176w (and potentially more) seem to omit environment variables set to an empty string in Run Script Phases (and potentially other places).

This was confirmed intentional by Apple: ⌘F for 40843445 in Xcode 10 Beta Release Notes (via Apple Developer Sign-In).

Member

jdhealy commented Sep 9, 2018

Xcode beta release 10L176w (and potentially more) seem to omit environment variables set to an empty string in Run Script Phases (and potentially other places).

This was confirmed intentional by Apple: ⌘F for 40843445 in Xcode 10 Beta Release Notes (via Apple Developer Sign-In).

@jdhealy jdhealy changed the title from Consider `XCODE_PRODUCT_BUILD_VERSION` alongside `EXPANDED_CODE_SIGN_IDENTITY` to address Xcode 10 beta. to Consider `XCODE_PRODUCT_BUILD_VERSION` alongside `EXPANDED_CODE_SIGN_IDENTITY` to address Xcode 10. Sep 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment