aws-c-cal: update from 0.9.10 to 0.9.13 - #2637
Conversation
f1b99d5 to
b4d7da9
Compare
b4d7da9 to
f51c332
Compare
bgilbert
left a comment
bgilbert
left a comment
There was a problem hiding this comment.
I'm not 100% certain I've understood the intended behavior of the new logic. I've flagged some inconsistencies but please double-check the result to make sure it properly handles all the cases.
| option( | ||
| 'byo_crypto', | ||
| type: 'feature', | ||
| value: 'disabled', |
There was a problem hiding this comment.
Personally I'm okay with this, given how it's used, but it is surprising that a feature doesn't default to auto.
| if ( | ||
| get_option('default_library') == 'shared' | ||
| and host_machine.system() == 'windows' | ||
| ) or host_machine.system() != 'windows' |
There was a problem hiding this comment.
| if ( | |
| get_option('default_library') == 'shared' | |
| and host_machine.system() == 'windows' | |
| ) or host_machine.system() != 'windows' | |
| if host_machine.system() != 'windows' or get_option('default_library') == 'shared' |
| aws_c_common_dep = dependency('aws-c-common') | ||
| libcrypto_dep = dependency( | ||
| 'libcrypto', | ||
| 'openssl', |
There was a problem hiding this comment.
Is this change correct? openssl implies both libcrypto and libssl, the actual TLS library.
| ncrypt = cc.find_library( | ||
| 'ncrypt', | ||
| required: host_machine.system() == 'windows', | ||
| required: false, |
There was a problem hiding this comment.
This, and the corresponding required: false for foundation, could theoretically produce a situation where byo_crypto is disabled and we also don't find the needed crypto library.
| 'libcrypto', | ||
| 'openssl', | ||
| version: '>=1.1', | ||
| required: not byo_crypto.allowed(), |
There was a problem hiding this comment.
This requires (and, further down the file, enables) OpenSSL on Windows and Darwin, contrary to the earlier comment.
| 'source/unix/opensslcrypto_hmac.c', | ||
| ) | ||
| else | ||
| error('Unsupported platform: ' + host_machine.system()) |
There was a problem hiding this comment.
Shouldn't we still error out in this case?
bgilbert
added
the
needs fixes
3 participants
Uh oh!
There was an error while loading. Please reload this page.