-
Notifications
You must be signed in to change notification settings - Fork 121
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
Fix zlib 1.2.12 vcpkg failure #2377
Conversation
How come CI has been passing even without this change over the last few weeks? It makes me wonder if we actually need this change in Embedded C, or if we aren't building the expected configuration in CI to hit the issue. |
FYI @danewalton, @CIPop - luckily this should only impact samples |
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.
Looks good (as long as it is actually needed to build the SDK, following our steps/CI to pass).
P.S. I just noticed our SDK version in the vcpkg.json file is out-dated (1.0.0), but we don't actually ship on vcpkg, but rather use it to get our dependencies, so it isn't crucial to keep in sync.
@ahsonkhan, we do also have versions in the C++ sdk, but we have two sets of files - first are the ones that are used for vcpkg export. Those should have version updated. But they keep it updated by using a placeholder, and then the script inserts the release version. Second set of |
Do we still need this change given that it appears that the fix is available in vcpkg? |
That's what I am wondering too. @antkmsft do we actually need this change? |
Yes, we need the fix, because baseline is at Line 4 in cd62e2b
And at that commit, |
Why not move C to a newer baseline? That baseline is back in April. |
@LarryOsterman, one of the dependencies, |
What is the plan to move the baseline forward? We can't remain stuck on a 6 month old package baseline for all packages. |
Was this already fixed? They've just changed the include paths (which was incorrect initially). |
Unfortunately, we can't easily pick same SHA that we picked for updating the C++ SDK (Azure/azure-sdk-for-cpp#4036), because
paho-mqtt
has a breaking change at that commit SHA (fatal error C1083: Cannot open include file: 'paho-mqtt/MQTTClient.h': No such file or directory
).So,
overrides
if not the final step, is definitely the first fix.