Skip to content

build cMocka lib manually to VCPKG #875

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
vhvb1989 merged 5 commits into from
Jul 1, 2020
Merged

build cMocka lib manually to VCPKG #875

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
69f7765
build cmocka lib manually to VCPKG
vhvb1989 Jul 1, 2020
c1b385f
Windows static lib name
vhvb1989 Jul 1, 2020
5bab442
Apply suggestions from code review
vhvb1989 Jul 1, 2020
20ed01f
format
vhvb1989 Jul 1, 2020
394c509
typo
vhvb1989 Jul 1, 2020
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
16 changes: 15 additions & 1 deletion cmake-modules/AddTestCMocka.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,21 @@ function(ADD_CMOCKA_TEST _TARGET_NAME)
endif()

if(DEFINED ENV{VCPKG_ROOT} OR DEFINED ENV{VCPKG_INSTALLATION_ROOT})
set(CMOCKA_LIB ${CMOCKA_LIBRARIES})
if(DEFINED ENV{VCPKG_ROOT})
set(VCPKG_PATH $ENV{VCPKG_ROOT})
endif()
if(DEFINED ENV{VCPKG_INSTALLATION_ROOT})
set(VCPKG_PATH "$ENV{VCPKG_INSTALLATION_ROOT}/")
Copy link
Contributor

@ahsonkhan ahsonkhan Jul 1, 2020

Choose a reason for hiding this comment

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

Is the / at the end of the string intentional? Why don't we have the same thing above, in set(VCPKG_PATH $ENV{VCPKG_ROOT})

Also, is that why we surround it in quotes?

Copy link
Member Author

@vhvb1989 vhvb1989 Jul 1, 2020

Choose a reason for hiding this comment

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

it is intentional.
In the above lines, (usually Linux and Windows) the VCPK_ROOT includes the / at the end of the path
For VCPK_INSTALLATION_ROOT (Mac) the / is missing, that's why we add it.

the quotes are just required in the second one to make the concatenation. Above lines we don't need to

endif()

# Patch for new VCPKG cmocka returning relative path
Copy link
Contributor

@ahsonkhan ahsonkhan Jul 1, 2020

Choose a reason for hiding this comment

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

Will we revert this once VCPKG issue is fixed (or maybe after the version pinning changes that @danieljurek was considering)?

Copy link
Member Author

@vhvb1989 vhvb1989 Jul 1, 2020

Choose a reason for hiding this comment

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

yeah, this is a temporal fix.
It is setting the path to cmocka libraries manually and hardcoded. This fix would work always while the name of the static library for cmocka doesn't change.

I am not sure if there is a bug for VCPKG, it might be that it used to be one and they fixed it and we were just making it work by luck wrongly before.

but, at any way, we need follow up on this

Copy link
Member

@danewalton-msft danewalton-msft Jul 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Note from my message thread on Teams. I notified the person which made the change that there was an issue here. The issue is open here and I'm working with her on the fix here

Copy link
Member Author

@vhvb1989 vhvb1989 Jul 1, 2020

Choose a reason for hiding this comment

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

thank you @danewalton .
That's good news.
I though cmocka could've done the change to work the same as when not using VCPKG.
when not working with VCPKG, cmake gets the path to the libs in a relative way, just like it is now now. That's why I thought that's how they wanted now.

So, as soon as VCPKG fix is merged, we can revert the changes here and it should work the same (and better without the hardcoded hot fix)

if(MSVC)
set(CMOCKA_STATIC_LIB "/lib/cmocka-static.lib")
else()
set(CMOCKA_STATIC_LIB "/lib/libcmocka-static.a")
Copy link
Contributor

@ahsonkhan ahsonkhan Jul 1, 2020

Choose a reason for hiding this comment

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

Question: Is the name of the .a file accurate? Is it supposed to be libcmocka-static?

Copy link
Member Author

@vhvb1989 vhvb1989 Jul 1, 2020

Choose a reason for hiding this comment

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

it is .a on Posix (lin/mac) and .lib on windows.

the expectation is that cmoka will use whatever is loaded on ${CMOCKA_LIBRARIES}. That variable is provided by VCPKG when cmocka is installed. Cmocka provides the scripts to VCPKG to generate this variable.

In the past, this variable was generated like:
degug;/path/to/VCPKG/{vcpkg packages installed path}/libraries;optimized;/path/to/VCPKG/{vcpkg packages installed path}/libraries
but cmocka made a change and now it looks like:
debug;/libraries;optimized;/libraries

So the prefix path to the libraries was removed from both, debug and optimized libs.

I am just manually building the value we used to have before

endif()
set(CMOCKA_PREFIX "${VCPKG_PATH}installed/${VCPKG_TARGET_TRIPLET}")
Copy link
Contributor

@ahsonkhan ahsonkhan Jul 1, 2020

Choose a reason for hiding this comment

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

Question: Does this need a directory separator?

Suggested change
set(CMOCKA_PREFIX "${VCPKG_PATH}installed/${VCPKG_TARGET_TRIPLET}")
set(CMOCKA_PREFIX "${VCPKG_PATH}/installed/${VCPKG_TARGET_TRIPLET}")

Copy link
Member Author

@vhvb1989 vhvb1989 Jul 1, 2020

Choose a reason for hiding this comment

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

no, VCPKG_PATH is expected to have the / at the end

set(CMOCKA_LIB "debug;${CMOCKA_PREFIX}/debug${CMOCKA_STATIC_LIB};optimized;${CMOCKA_PREFIX}${CMOCKA_STATIC_LIB}")
Copy link
Contributor

@ahsonkhan ahsonkhan Jul 1, 2020

Choose a reason for hiding this comment

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

Question: How did you know this is what we need to set CMOCKA_LIB to? Will we always have debug?

I am trying to understand what this means.

Copy link
Member Author

@vhvb1989 vhvb1989 Jul 1, 2020

Choose a reason for hiding this comment

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

no,
this is how a package tells VCPKG where to find libraries for debug and for optimized.
The string looks like degug;path;optimized;path

Then, depending on what cmake is building, it will pick debug or optimized.

Then one on optimized does not have the debug/

else()
set(CMOCKA_LIB cmocka)
endif()
Expand Down