Skip to content

Fix building NativeShims on OSX #109

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.

Sign up for GitHub

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 2 commits into from
Jun 26, 2024
Merged

Fix building NativeShims on OSX #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
60898c4
fixes #76
AdamVe Jun 24, 2024
ad18582
remove unused files
AdamVe Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Yubico.NativeShims/build-macOS.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@ git pull
vcpkg x-update-baseline
popd

\cmake \
cmake \
-S . \
-B ./build64 \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_TOOLCHAIN_FILE=$VCPKG_INSTALLATION_ROOT/scripts/buildsystems/vcpkg.cmake \
-DVCPKG_TARGET_TRIPLET=x64-osx

-DVCPKG_TARGET_TRIPLET=x64-osx \
-DCMAKE_OSX_ARCHITECTURES=x86_64
Comment on lines 14 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At one point we used custom toolchain files. It had set the CMAKE_OSX_ARCHITECTURE option among others. We probably unintentionally stopped using them when we started using VCPKG.

I'd recommend taking a look at the ones we have in the cmake/ subfolder. We should probably either start using them as overlays (I think that's the term vcpkg uses) or get rid of them.

Either way - would probably be good to check to see if there are other settings that were used once upon a time that might be worth reintroducing.

I'd also double check to see if VCPKG_TARGET_TRIPLET doesn't already set/imply CMAKE_OSX_ARCHITECTURES if you haven't done so already.

Copy link
Member Author

@AdamVe AdamVe Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points! I did some investigations here and this is what I found out:

Although the triplets set VCPKG_OSX_ARCHITECTURES to correct values, those are not used at all on OSX leaving CMAKE_OSX_ARCHITECTURES undefined, which will fail linkage for specific build/host/target combinations. Explicitly defining the value for each target platform fixes the issue.

I was not aware of the cmake/osx*cmake files, currently they are not used and in the original commit they served as the main toolchain files. They could be reused through VCPKG_CHAINLOAD_TOOLCHAIN_FILE but I don’t see any advantage to just using -DCMAKE_OSX_ARCHITECTURES as vcpkg is used now.

See also that the cmake/osx*cmake files do cache the specific CMAKE_OSX_ARCHITECTURES value (functionally same effect as using the explicit -D).

My way forward would be to use -DCMAKE_OSX_ARCHITECTURES=arch and delete the unused cmake files. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that sounds quite reasonable to me. Thanks Adam!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for simplifying and deleting unused files. Thanks @AdamVe !


cmake --build ./build64
mkdir osx-x64
Expand Down
14 changes: 0 additions & 14 deletions Yubico.NativeShims/cmake/osx-arm64.toolchain.cmake
View file
Open in desktop

This file was deleted.

14 changes: 0 additions & 14 deletions Yubico.NativeShims/cmake/osx-x64.toolchain.cmake
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion Yubico.NativeShims/ssl.bignum.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ typedef void* Native_BIGNUM;

Native_BIGNUM
NATIVEAPI
Native_BN_new()
Native_BN_new(void)
{
return BN_new();
}
Expand Down
2 changes: 1 addition & 1 deletion Yubico.NativeShims/ssl.cmac.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ typedef void* Native_EVP_MAC_CTX;

Native_EVP_MAC_CTX
NATIVEAPI
Native_CMAC_EVP_MAC_CTX_new()
Native_CMAC_EVP_MAC_CTX_new(void)
{
EVP_MAC *mac = NULL;
EVP_MAC_CTX *macCtx = NULL;
Expand Down
2 changes: 1 addition & 1 deletion Yubico.NativeShims/ssl.gcmevp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ typedef void* Native_EVP_CIPHER_CTX;

Native_EVP_CIPHER_CTX
NATIVEAPI
Native_EVP_CIPHER_CTX_new()
Native_EVP_CIPHER_CTX_new(void)
{
return EVP_CIPHER_CTX_new();
}
Expand Down