-
Notifications
You must be signed in to change notification settings - Fork 515
Update MD5 generation to CryptoKit Insecure.MD5 #616
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
Conversation
PINRemoteImage.podspec
Outdated
| cs.osx.deployment_target = osx_deployment | ||
| cs.source_files = 'Source/Classes/**/*.{h,m}' | ||
| cs.public_header_files = 'Source/Classes/**/*.h' | ||
| cs.source_files = 'Source/*/**/*.{swift,h,m}' |
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 the first */ can be removed here since ** will include all directories?
PINRemoteImage.podspec
Outdated
| cs.source_files = 'Source/Classes/**/*.{h,m}' | ||
| cs.public_header_files = 'Source/Classes/**/*.h' | ||
| cs.source_files = 'Source/*/**/*.{swift,h,m}' | ||
| cs.public_header_files = 'Source/*/**/*.h' |
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.
Same here, I think this can be 'Source/**/*.h'
| PROVISIONING_PROFILE_SPECIFIER = ""; | ||
| SDKROOT = appletvos; | ||
| SKIP_INSTALL = YES; | ||
| SWIFT_OPTIMIZATION_LEVEL = "-Onone"; |
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.
Is this only for the debug build? We don't want this to be set for release versions I don't think.
| PROVISIONING_PROFILE_SPECIFIER = ""; | ||
| SDKROOT = appletvos; | ||
| SWIFT_OBJC_BRIDGING_HEADER = "Tests/PINRemoteImage-tvOSTests-Bridging-Header.h"; | ||
| SWIFT_OPTIMIZATION_LEVEL = "-Onone"; |
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.
Same question here.
| MODULEMAP_FILE = Source/PINRemoteImage.modulemap; | ||
| PRODUCT_BUNDLE_IDENTIFIER = com.pinterest.PINRemoteImage; | ||
| SKIP_INSTALL = YES; | ||
| SWIFT_OPTIMIZATION_LEVEL = "-Onone"; |
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.
And here 😅
Tests/PINRemoteImageTests.m
Outdated
| return request; | ||
| }]; | ||
| }; | ||
|
|
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 if we avoid using the locks we may create flakiness in a different way :( Does calling [self waitForExpectationsWithTimeout:[self timeoutTimeInterval] handler:nil]; after calling setRequestConfiguration: solve the flakiness?
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.
Oh man, I'm so sorry this is designed this way 😅
I think we need to add a new API:
setRequestConfiguration:completion (which the old setRequestConfiguration: can call with a nil completion). Then we can call the new API in the test method with a new expectation that is fulfilled in the completion block. We'll then wait for just that expectation to be fulfilled before calling downloadImageWithURL:…
|
Closing since we decided not to continue with MD5, extracted the stability changes unto #618 |
When setting minimum deployment target to iOS >= 13, a build error is generated about CC_MD5 being deprecated. This PR updates the usages to CryptoKit's
Insecure.MD5.The functionality is the same, but through the usage of a newer API, now the framework builds for iOS >= 13