Skip to content
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

[jansson] Fix UWP build #24466

Merged
merged 9 commits into from
Apr 29, 2022
Merged

[jansson] Fix UWP build #24466

merged 9 commits into from
Apr 29, 2022

Conversation

Thomas1664
Copy link
Contributor

@Thomas1664 Thomas1664 commented Apr 28, 2022

Describe the pull request

  • What does your PR fix?

    Baseline regression:

    The following DLLs do not have the App Container bit set:
       D:/packages/jansson_arm-uwp/debug/bin/jansson_d.dll
       D:/packages/jansson_arm-uwp/bin/jansson.dll
    
  • Which triplets are supported/not supported? Have you updated the CI baseline?

    Unchanged

  • Does your PR follow the maintainer guide?

    Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    Yes

@Neumann-A
Copy link
Contributor

The error means somebody is illegally modify build flags without taking toolchain settings into account

@Thomas1664
Copy link
Contributor Author

Thomas1664 commented Apr 28, 2022

The error means somebody is illegally modify build flags without taking toolchain settings into account

@Neumann-A The only 2 added flags are -Wl, --default-symver (or -Wl,--version-script) Are these problematic?

@Neumann-A
Copy link
Contributor

One line completly overrides CMAKE_SHARDED_LINKER_FLAGS that is why it is failing.

@Thomas1664
Copy link
Contributor Author

Already upstreamed patch: akheron/jansson#610

@Thomas1664 Thomas1664 changed the title [jansson] (Hopefully) Fix UWP build [jansson] Fix UWP build Apr 28, 2022
@@ -342,7 +342,7 @@ if(JANSSON_BUILD_SHARED_LIBS)
)
list(REMOVE_ITEM CMAKE_REQUIRED_LIBRARIES "-Wl,--version-script,${CMAKE_CURRENT_BINARY_DIR}/jansson.sym")
if (VSCRIPT_WORKS)
Copy link
Contributor

Choose a reason for hiding this comment

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

even though this is upstream it probably should read if(VSCRIPT_WORKS AND NOT MSVC)

Copy link
Contributor Author

@Thomas1664 Thomas1664 Apr 28, 2022

Choose a reason for hiding this comment

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

Why? Before this, upstream checks if --default-symver works. I don't think we should patch this just to safe a few seconds configure time on Windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that cl only warns about unknown flags and doesn't error on them like gcc/clang. So it always works on windows but in reality it never works.

else()
set(JANSSON_BUILD_SHARED_LIBS OFF)
endif()
string(COMPARE EQUAL "${VCPKG_CRT_LINKAGE}" "static" JANSSON_STATIC_CRT)
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading the CMakeLists.txt this is probably not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if we set the same flags, but even if we do, we should keep this in case upstream decides to do additional things with JANSSON_STATIC_CRT

@BillyONeal BillyONeal merged commit 585ff44 into microsoft:master Apr 29, 2022
@BillyONeal
Copy link
Member

You beat me to it, thank you!

BillyONeal added a commit to BillyONeal/vcpkg that referenced this pull request Apr 29, 2022
…,pqp,smpeg2] Build fixes 2022-04-28

These results are from the most recent CI run: https://dev.azure.com/vcpkg/public/_build/results?buildId=71465

PASSING, REMOVE FROM FAIL LIST: aubio:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).

I also did some investigation as to why aubio:arm-uwp didn't pass. Turns out, it's because aubio depends on ffmpeg, which failed to build because it depends on libvpx, which we never fixed for UWP following the VS2022 update. See also https://developercommunity.visualstudio.com/t/MicrosoftVisualStudioComponentVCTool/10002207?space=62&scope=follow&sort=newest

PASSING, REMOVE FROM FAIL LIST: freetype-gl:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).

I also checked freetype-gl:arm-uwp, but it's blocked by glew which is blocked by opengl which appears to not be a thing on arm.

PASSING, REMOVE FROM FAIL LIST: intelrdfpmathlib:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).

The arm-uwp version of this emits errors that look like source issues; I blocked arm&windows with a supports expression:
D:\buildtrees\intelrdfpmathlib\src\athLib20U2-d2a8954428.clean\LIBRARY\src\bid_functions.h(3113): error C2719: 'x': formal parameter with requested alignment of 16 won't be aligned

PASSING, REMOVE FROM FAIL LIST: libbson:arm-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: libbson:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: libtcod:arm-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: libtcod:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: lmdb:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).

arm-uwp failed with again what looks like a source issue:
mdb.c.obj : error LNK2001: unresolved external symbol __tls_used
mdb.c.obj : error LNK2001: unresolved external symbol _mdb_tls_cbp

PASSING, REMOVE FROM FAIL LIST: metis:arm-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: metis:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: pqp:arm-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: pqp:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: smpeg2:arm-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: smpeg2:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).

I also checked Linux and it says
  Could NOT find ibverbs (missing: IBVERBS_INCLUDE_DIRS IBVERBS_LIBRARIES)
which may be vcpkg's fault so I left that ci.baseline.txt skip alone.

REGRESSION: jansson:arm-uwp failed with POST_BUILD_CHECKS_FAILED. If expected, add jansson:arm-uwp=fail to C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: jansson:x64-uwp failed with POST_BUILD_CHECKS_FAILED. If expected, add jansson:x64-uwp=fail to C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt.

Already fixed by microsoft#24466
@BillyONeal
Copy link
Member

I agree with @Neumann-A 's PR comments but this change fixes CI runs. @Thomas1664 Can you apply those suggestions in the upstream version of the change?

BillyONeal added a commit that referenced this pull request Apr 30, 2022
…,pqp,smpeg2] Build fixes 2022-04-28 (#24470)

* [libvpx,lmdb,aubio,freetype-gl,intelrdfpmathlib,libbson,libtcod,metis,pqp,smpeg2] Build fixes 2022-04-28

These results are from the most recent CI run: https://dev.azure.com/vcpkg/public/_build/results?buildId=71465

PASSING, REMOVE FROM FAIL LIST: aubio:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).

I also did some investigation as to why aubio:arm-uwp didn't pass. Turns out, it's because aubio depends on ffmpeg, which failed to build because it depends on libvpx, which we never fixed for UWP following the VS2022 update. See also https://developercommunity.visualstudio.com/t/MicrosoftVisualStudioComponentVCTool/10002207?space=62&scope=follow&sort=newest

PASSING, REMOVE FROM FAIL LIST: freetype-gl:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).

I also checked freetype-gl:arm-uwp, but it's blocked by glew which is blocked by opengl which appears to not be a thing on arm.

PASSING, REMOVE FROM FAIL LIST: intelrdfpmathlib:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).

The arm-uwp version of this emits errors that look like source issues; I blocked arm&windows with a supports expression:
D:\buildtrees\intelrdfpmathlib\src\athLib20U2-d2a8954428.clean\LIBRARY\src\bid_functions.h(3113): error C2719: 'x': formal parameter with requested alignment of 16 won't be aligned

PASSING, REMOVE FROM FAIL LIST: libbson:arm-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: libbson:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: libtcod:arm-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: libtcod:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: lmdb:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).

arm-uwp failed with again what looks like a source issue:
mdb.c.obj : error LNK2001: unresolved external symbol __tls_used
mdb.c.obj : error LNK2001: unresolved external symbol _mdb_tls_cbp

PASSING, REMOVE FROM FAIL LIST: metis:arm-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: metis:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: pqp:arm-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: pqp:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: smpeg2:arm-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).
PASSING, REMOVE FROM FAIL LIST: smpeg2:x64-uwp (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).

I also checked Linux and it says
  Could NOT find ibverbs (missing: IBVERBS_INCLUDE_DIRS IBVERBS_LIBRARIES)
which may be vcpkg's fault so I left that ci.baseline.txt skip alone.

REGRESSION: jansson:arm-uwp failed with POST_BUILD_CHECKS_FAILED. If expected, add jansson:arm-uwp=fail to C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: jansson:x64-uwp failed with POST_BUILD_CHECKS_FAILED. If expected, add jansson:x64-uwp=fail to C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt.

Already fixed by #24466

* dos2unix the patch

* :dos2unix the other patches too
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants