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.

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

Jump to bottom

[3fd|python3] switch to vcpkg-msbuild #33026

Merged
merged 9 commits into from Aug 9, 2023
Merged

[3fd|python3] switch to vcpkg-msbuild #33026

merged 9 commits into from Aug 9, 2023

Conversation

Neumann-A
Copy link
Contributor

@Neumann-A Neumann-A commented Aug 8, 2023 •
edited

  • make 3fd use vcpkg-msbuild
  • make python use vcpkg-msbuild
  • fix python cross builds on windows

dg0yt
dg0yt reviewed Aug 8, 2023
View reviewed changes
ports/python3/portfile.cmake
@@ -94,11 +95,15 @@ if(VCPKG_TARGET_IS_WINDOWS)
find_library(SQLITE_DEBUG NAMES sqlite3 PATHS "${CURRENT_INSTALLED_DIR}/debug/lib" NO_DEFAULT_PATH)
find_library(SSL_RELEASE NAMES libssl PATHS "${CURRENT_INSTALLED_DIR}/lib" NO_DEFAULT_PATH)
find_library(SSL_DEBUG NAMES libssl PATHS "${CURRENT_INSTALLED_DIR}/debug/lib" NO_DEFAULT_PATH)
list(APPEND add_libs_rel "${BZ2_RELEASE};${EXPAT_RELEASE};${FFI_RELEASE};${LZMA_RELEASE};${SQLITE_RELEASE}")
list(APPEND add_libs_dbg "${BZ2_DEBUG};${EXPAT_DEBUG};${FFI_DEBUG};${LZMA_DEBUG};${SQLITE_DEBUG}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this benefit from x_vcpkg_pkgconfig_get_modules()?

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 could rather add DEPENDENT_PKGCONFIG to vcpkg_msbuild_install but didn't want to go there yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these libs duplicated in props file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ultimate cleanup plan would be to remove openssl.props.in and python_vcpkg.props.in completely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Hoikas I assume the why is in response of dropping the props files as ultimate cleanup. Simply said no portfile should require a direct interaction with build system internals. In the case of MSBuild it is the injection of props files which every port would have needed to to manually. It is simply better to have an abstraction around that than having to deal with msbuild shenanigans.

@dg0yt dg0yt mentioned this pull request Aug 8, 2023
Draft
7 tasks
@Adela0814 Adela0814 added the category:port-bug The issue is with a library, which is something the port should already support label Aug 8, 2023
@Adela0814
Copy link
Contributor

CI run failed:
Error:

    28>D:\buildtrees\python3\x86-windows-rel\PCbuild\obj\311amd64_Release\_freeze_module\_freeze_module.obj : fatal error LNK1112: module machine type 'x64' conflicts with target machine type 'x86' [D:\buildtrees\python3\x86-windows-rel\PCbuild\_freeze_module.vcxproj]
    28>Done Building Project "D:\buildtrees\python3\x86-windows-rel\PCbuild\_freeze_module.vcxproj" (Build target(s)) -- FAILED.
     1>Done Building Project "D:\buildtrees\python3\x86-windows-rel\PCbuild\pcbuild.proj" (Rebuild target(s)) -- FAILED.

Details:
build-x86-windows-rel-out.log

@Adela0814 Adela0814 marked this pull request as draft August 8, 2023 09:47
@Neumann-A
Copy link
Contributor Author

CI run failed:

Yeah I know. Something wrong with msbuild not selecting the right configuration for some reason. The command line has /p:Configuration=Release and /p:Platform=Win32 so I don't quite know why it is selecting x64 instead

@Neumann-A
Copy link
Contributor Author

hmm even E:\vcpkg\buildtrees\python3\x86-windows-rel>msbuild E:/vcpkg/buildtrees/python3/x86-windows-rel/PCbuild/pcbuild.proj -p:Platform=Win32 doesn't call the correct compiler from vcpkg env --triplet x86-windows

@Neumann-A
Copy link
Contributor Author

Hmm there is a bug in python3 which builds an executable for the host instead the target which is why the cross builds fail here (see #31727 (comment))

@Neumann-A Neumann-A changed the title [python3] switch to vcpkg-msbuild [3fd|python3] switch to vcpkg-msbuild Aug 8, 2023
@Neumann-A Neumann-A marked this pull request as ready for review August 8, 2023 14:22
dg0yt
dg0yt reviewed Aug 9, 2023
View reviewed changes
ports/3fd/portfile.cmake Outdated
@@ -13,18 +13,16 @@ vcpkg_from_github(
)

# Build:
if (VCPKG_CMAKE_SYSTEM_NAME STREQUAL "WindowsStore") # UWP:
if (VCPKG_TARGET_IS_UWP) # UWP:
vcpkg_install_msbuild(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this to use vcpkg_msbuild_install, too?

Copy link
Contributor Author

@Neumann-A Neumann-A Aug 9, 2023 •
edited

Choose a reason for hiding this comment

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

Hmm would have assumed CI to catch that but I see 3fd:x64-uwp: cascade. So the port once build for uwp but no longer does.

Reason: poco is only supported on '!uwp', which does not match x64-uwp.

@Adela0814 Adela0814 added the info:reviewed Pull Request changes follow basic guidelines label Aug 9, 2023
@Osyotr
Copy link
Contributor

Osyotr commented Aug 9, 2023 •
edited

So the official python installation does not contain _freeze_module.exe. This tool is used only at build time thus can be removed from the installed tree. Is it bad that it's built with real host arch?

@Neumann-A
Copy link
Contributor Author

Neumann-A commented Aug 9, 2023 •
edited

Is it bad that it's built with real host arch?

Yes since that is against vcpkg policy. vcpkg only builds for the target triplet nothing else.

This tool is used only at build time thus can be removed from the installed tree

No it cannot since it is required to run for cross builds to generate some headers/scripts if I understand what it is doing correctly. I moved it to manual-tools/python3 so that it won't get picked up by CMake or whatever else might go looking for .exe files.

The build of that project could probably be suppressed for VCPKG_CROSSCOMPILING somehow but I did not look into how to achieve that and what might happen to dependent projects.

@JavierMatosD JavierMatosD merged commit 2727697 into microsoft:master Aug 9, 2023
15 checks passed
@Neumann-A Neumann-A deleted the python3_vcpkg_msbuild branch August 9, 2023 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hoikas Hoikas Hoikas left review comments

@Osyotr Osyotr Osyotr left review comments

@dg0yt dg0yt dg0yt left review comments

@Adela0814 Adela0814 Adela0814 approved these changes

Assignees

@Adela0814 Adela0814

Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Neumann-A @Adela0814 @Osyotr @Hoikas @dg0yt @JavierMatosD