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

[wxwidgets] Fix #4756 #13361

Merged
merged 14 commits into from Feb 9, 2021
Merged

Conversation

ras0219
Copy link
Contributor

@ras0219 ras0219 commented Sep 5, 2020

Alternative approach compared to #13023

I'm able to successfully compile https://docs.wxwidgets.org/trunk/overview_helloworld.html with the following CMakeLists.txt:

cmake_minimum_required(VERSION 3.17)
project(foo)

add_executable(foo foo.cpp)

find_package(wxWidgets REQUIRED COMPONENTS core qa html xml aui adv stc webview base scintilla)
include(${wxWidgets_USE_FILE})
target_link_libraries(foo ${wxWidgets_LIBRARIES})

The source also required modification to add wxIMPLEMENT_WXWIN_MAIN_CONSOLE since our built copy of wxwidgets targets GUIs.

@ras0219 ras0219 marked this pull request as draft September 5, 2020 00:52
@LilyWangL LilyWangL added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:internal This PR or Issue was filed by the vcpkg team. labels Sep 7, 2020
@marekr
Copy link
Contributor

marekr commented Sep 14, 2020

This seems like a very awkward hack with removing WXUSINGDLL and replacing it with #if 1 or 0.........

You are supposed to define WXUSINGDLL in your application. Your code would never compile outside VCPKG and this encourages people to make that (minor) error.

Edit:
Normally in cmake land, FindWxWidgets.cmake is supposed to append WXUSINGDLL automatically on Windows. I believe the stock findwxwidgets cmake module is where your problems lie.

We at the KiCad project have our own corrected copy of FindWxwidgets that fixes compatibility with vcpkg and have no need to violate the source of wxwidgets.
https://gitlab.com/kicad/code/kicad/-/blob/master/CMakeModules/FindwxWidgets.cmake#L513

@ras0219
Copy link
Contributor Author

ras0219 commented Nov 19, 2020

@marekr I'm really happy you've been able to work around these issues with a custom file!

However, in upstream vcpkg,

  1. We really don't want to force every user to write a custom FindwxWidgets.cmake
  2. We have to support more buildsystems than just CMake

Because vcpkg always deploys exactly one version of wxwidgets (either DLL or static) we can statically encode the linkage in the package, which removes a source of error on the consumption side. If the user correctly set the macro in their project, there will be no observable difference in the build. This is a technique we apply broadly across vcpkg for libraries to handle ABI-affecting macros.

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@NancyLi1013 NancyLi1013 left a comment

Choose a reason for hiding this comment

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

The control file for wxchartdir has been delated. vcpkg.json seems missing in this PR.

@NancyLi1013
Copy link
Contributor

The failure on osx:

There should be no bin\ directory in a static build, but /Users/vagrant/Data/packages/wxwidgets_x64-osx/bin is present.
There should be no debug\bin\ directory in a static build, but /Users/vagrant/Data/packages/wxwidgets_x64-osx/debug/bin is present.
If the creation of bin\ and/or debug\bin\ cannot be disabled, use this in the portfile to remove them

    if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
        file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/bin" "${CURRENT_PACKAGES_DIR}/debug/bin")
    endif()

@ras0219 ras0219 force-pushed the dev/roschuma/wxwidgets branch 2 times, most recently from 3b628e8 to a4081f5 Compare January 6, 2021 16:47
@ras0219 ras0219 marked this pull request as ready for review January 8, 2021 03:25
port_versions/baseline.json Outdated Show resolved Hide resolved
port_versions/w-/wxwidgets.json Outdated Show resolved Hide resolved
ports/wxwidgets/vcpkg.json Show resolved Hide resolved
ports/wxwidgets/portfile.cmake Show resolved Hide resolved
@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Feb 8, 2021
@ras0219-msft ras0219-msft merged commit ea11604 into microsoft:master Feb 9, 2021
@ras0219-msft
Copy link
Contributor

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wxwidgets installed but not detected by FindwxWidgets.cmake
6 participants