Skip to content
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

Using Fastlane::Actions::VerifyXcodeAction for verify_app_cert #361

Merged
merged 3 commits into from Nov 9, 2019

Conversation

@Kesin11
Copy link
Contributor

Kesin11 commented Oct 26, 2019

fix #360

Current version of xcode-install (v2.6.1) can not install Xcode 10.x due to verify certificate step.

In #338 @salmanasiddiqui suggested using VerifyXcodeAction instead of VerifyBuildAction.
I tried his suggest and it works.

I confirmed in my forked repositories that this code fix can actually install Xcode 10.x.
https://github.com/Kesin11/xcode-install/runs/276027924

However, I know this fix is not better because it will be more dependent on fastlane. In addition, additional logs that from VerifyXcodeAction have been displayed.

Another choice is only copy accepted codesign sets from verify_xcode.rb.
Which choice should I take?

@Vyazovoy

This comment has been minimized.

Copy link

Vyazovoy commented Oct 30, 2019

It is reasonable to keep verification logic in one well maintained place (Fastlane). Also your PR makes code easier to understand 👍

apple_team_identifier_result = cert_info['team_identifier'] == TEAM_IDENTIFIER
apple_authority_result = cert_info['authority'].include?(AUTHORITY)
apple_team_identifier_result && apple_authority_result
begin

This comment has been minimized.

Copy link
@marcomorain

marcomorain Oct 30, 2019

Collaborator

You can remove a level of nesting here, since a def is an implicit begin:

https://forum.upcase.com/t/implicit-begin/205

🤷‍♂

This comment has been minimized.

Copy link
@milch

milch Oct 30, 2019

Collaborator

+1, probably not a a bad idea

This comment has been minimized.

Copy link
@Kesin11

Kesin11 Oct 31, 2019

Author Contributor

I didn't know this syntax. Thanks!

@milch

This comment has been minimized.

Copy link
Collaborator

milch commented Oct 30, 2019

@marcomorain I updated the code in-place, if you could take a look to make sure everything is alright, that'd be great

@Kesin11 Thanks for the fix!

@KirkMartinez

This comment has been minimized.

Copy link

KirkMartinez commented Oct 31, 2019

This may not be a complete fix. I now get this message during validation /Applications/Xcode-10.2.1.app: unsealed contents present in the bundle root

@Kesin11

This comment has been minimized.

Copy link
Contributor Author

Kesin11 commented Nov 3, 2019

@KirkMartinez In my newest result of installing Xcode shows Xcode 10.2.1 can be installed 👍
My fork repository includes this patch.
https://github.com/Kesin11/xcode-install/runs/284517840

@Kesin11

This comment has been minimized.

Copy link
Contributor Author

Kesin11 commented Nov 6, 2019

Please someone review or merge it.
@milch @marcomorain Can you merge this pull-request?

@milch

This comment has been minimized.

Copy link
Collaborator

milch commented Nov 9, 2019

Apologies, I think this is probably fine to merge. In my testing installing Xcode 10.2.1 also worked, so I think there must be some other reason why it was not working for @KirkMartinez

@milch milch merged commit 38957ad into xcpretty:master Nov 9, 2019
@milch

This comment has been minimized.

Copy link
Collaborator

milch commented Nov 9, 2019

@Kesin11 Your end to end test setup looks pretty sweet, do you want to submit a PR for that as well?

@Kesin11

This comment has been minimized.

Copy link
Contributor Author

Kesin11 commented Nov 9, 2019

@milch Thanks for merge!
OK I'll submit a PR. I'm sure that test code will help developing xcode-install.
But of course, it needs AppleID that has disabled 2FA. Someone have to prepare AppleID.

@joshdholtz

This comment has been minimized.

Copy link
Member

joshdholtz commented Nov 14, 2019

Popping a new release with this in it in a bit!

@joshdholtz

This comment has been minimized.

Copy link
Member

joshdholtz commented Nov 14, 2019

Released in 2.6.3 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.