[wxwidgets] fix wxWidgets_LIBRARIES for windows, incl. static builds by jjYBdx4IL · Pull Request #23980 · microsoft/vcpkg

jjYBdx4IL · 2022-04-04T12:55:43Z

What does your PR fix?

fix wxWidgets_LIBRARIES for windows static builds (add missing comctl32 etc.)
add wxbase to list of debug libs

Which triplets are supported/not supported? Have you updated the CI baseline?

only changed windows stuff, except for the added wxbase debug lib. Is there any convenient CI system available to test the other platforms/triplets before creating a pull request?

Does your PR follow the maintainer guide?

Yes. Think so.

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

Yes

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/
No.

ghost · 2022-04-04T12:55:55Z

All CLA requirements met.

…tl32 etc.) * add wxbase to list of debug libs

cenit · 2022-04-04T20:55:46Z

appending to a file provided by vcpkg itself is not so nice. isn’t it better to make it a configurable file and copy it through a configure_file “@only” with appropriate variables?

jjYBdx4IL · 2022-04-04T21:08:55Z

appending to a file provided by vcpkg itself is not so nice. isn’t it better to make it a configurable file and copy it through a configure_file “@only” with appropriate variables?

Indeed. I wasn't aware of that mechanism, yet.

dg0yt

Basing the link libraries on globbing will break most static non-MSVC cases because the order of libraries matters for these linkers.

Unfortunately, we lack some CI coverage here, due to WxWidgets' dependency on gtk3 for Linux. There must be manual testing on x64-linux with some port that uses this wrapper.
(wxwidgets:x64-linux was fixed only recently, and a immediate regression must be avoided.)

ports/wxwidgets/portfile.cmake

ports/wxwidgets/vcpkg-cmake-wrapper.cmake

jjYBdx4IL · 2022-04-06T19:28:08Z

Better? Works for me [tm] on Linux64, OSX64, Win64.

github-actions

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated

git add -u && git commit
git checkout 01d6f6ff1e5332b926099f0c23bda996940ad4e8 -- versions
./vcpkg x-add-version --all

Diff

diff --git a/versions/w-/wxwidgets.json b/versions/w-/wxwidgets.json
old mode 100755
new mode 100644

LilyWangLL · 2022-04-07T06:00:03Z

@dg0yt Could you please review this PR again? Thanks in advance.

jjYBdx4IL · 2022-04-07T09:03:21Z

Unfortunately, we lack some CI coverage here, due to WxWidgets' dependency on gtk3 for Linux. There must be manual testing on x64-linux with some port that uses this wrapper. (wxwidgets:x64-linux was fixed only recently, and a immediate regression must be avoided.)

where is the test case? Can we have a test case repository please?

jjYBdx4IL · 2022-04-07T10:07:32Z

Problem is, the MSVC port currently is totally broken for me. Static builds outright fail compilation because debug and release code is mixed. And even if I set dynamic linking in my custom triplet for wxWidgets, it won't pull debug libs and instead link against the release libs which also won't be copied into the output directory.

dg0yt · 2022-04-07T10:10:40Z

I wish I could name a test port... It is probably easier to create a minimal testproject yourself.

@talregev Can you test this PR with your Linux project?

dg0yt · 2022-04-07T10:30:26Z

Problem is, the MSVC port currently is totally broken for me. Static builds outright fail compilation because debug and release code is mixed. And even if I set dynamic linking in my custom triplet for wxWidgets, it won't pull debug libs and instead link against the release libs which also won't be copied into the output directory.

The port has some history of problems and draft PRs. Some outside triggers have been fixed IIRC, and it would be good to resolve the remaining issues. CC @JackBoosY

Maybe it would be good to (at least temporarily) add a vcpkg-ci-wxwidgets test port that tries to build a minimal example app in CI.

I'm willing to test, too. But wxwidgets is not very high on my list, and I use MSVC only in CI.

jjYBdx4IL · 2022-04-07T10:58:45Z

Maybe it would be good to (at least temporarily) add a vcpkg-ci-wxwidgets test port that tries to build a minimal example app in CI.

How would that work without gtk3?

How about one (optional) repository on github for each port and which runs against github workflows that allow for more customization?

talregev · 2022-04-07T11:11:04Z

I wish I could name a test port... It is probably easier to create a minimal testproject yourself.

@talregev Can you test this PR with your Linux project?

@dg0yt
I will love to test this PR with the open source project that I am working on,
But currently the state of wxwidgets for my project is that it cannot compile due some of linking problem bug I mention in previous wxwidgets PR: #23765 (comment)
@JackBoosY promise to fix them and I am still waiting.
Can you solve the problem, then the project can compile with wxwidgets then @jjYBdx4IL can rebase to master, then I can check his PR.

Sounds reasonable?

dg0yt · 2022-04-07T11:16:28Z

Maybe it would be good to (at least temporarily) add a vcpkg-ci-wxwidgets test port that tries to build a minimal example app in CI.

How would that work without gtk3?

It wouldn't work now in CI, but a) we could use it locally, and b) there is a gtk3 PR.

How about one (optional) repository on github for each port and which runs against github workflows that allow for more customization?

When you want to talk about "for each port" or about more GH workflows, you are leaving the scope of this PR. I would avoid it for now.

jjYBdx4IL · 2022-04-07T11:40:29Z

@dg0yt how about simply avoiding the external dependencies and using the builtin ones? Maybe enable the external dependencies only through a feature?

jjYBdx4IL · 2022-04-07T11:43:22Z

When you want to talk about "for each port" or about more GH workflows, you are leaving the scope of this PR. I would avoid it for now.

Well, we could start with a single, simple repo for wxwidgets test cases. Having a well-defined test suite would make all of this A LOT more efficient. And in future one could simply refuse bug reports without attached test cases....

jjYBdx4IL · 2022-04-07T17:16:20Z

I moved the code to a cmake file in my own repository. If someone needs it, here it is. Maybe include it in a macro that's optional to execute after find_package?

# fix vcpkg wxWidgets deps
# (include after find_package(wxWidgets...))
set(_wxWidgets_LIBRARIES)
foreach(entry ${wxWidgets_LIBRARIES})

    get_filename_component(LIB_NAME_RELEASE ${entry} NAME)
    get_filename_component(LIB_NAME_WLE_RELEASE ${entry} NAME_WLE)

    if(IS_ABSOLUTE ${entry} AND ${LIB_NAME_WLE_RELEASE} MATCHES "^wx[a-z0-9]+u(|_.*)\$")
        if("@VCPKG_LIBRARY_LINKAGE@" STREQUAL "static")
            add_library(${LIB_NAME_WLE_RELEASE} STATIC IMPORTED)
        else()
            add_library(${LIB_NAME_WLE_RELEASE} SHARED IMPORTED)
        endif()

        list(APPEND _wxWidgets_LIBRARIES ${LIB_NAME_WLE_RELEASE})

        string(REGEX REPLACE "(wx[a-z0-9]+u)([_.].*)\$" "\\1d\\2" LIB_NAME_DEBUG "${LIB_NAME_RELEASE}")
        get_filename_component(LIB_NAME_WLE_DEBUG ${LIB_NAME_DEBUG} NAME_WLE)

        if("@VCPKG_LIBRARY_LINKAGE@" STREQUAL "static")
            set_target_properties(${LIB_NAME_WLE_RELEASE} PROPERTIES
                IMPORTED_LOCATION_RELEASE "${wxWidgets_ROOT_DIR}/lib/${LIB_NAME_RELEASE}"
                IMPORTED_LOCATION_DEBUG "${wxWidgets_ROOT_DIR}/debug/lib/${LIB_NAME_DEBUG}"
            )
        else()
            set_target_properties(${LIB_NAME_WLE_RELEASE} PROPERTIES
                IMPORTED_LOCATION_RELEASE "${wxWidgets_ROOT_DIR}/bin/${LIB_NAME_RELEASE}"
                IMPORTED_LOCATION_DEBUG "${wxWidgets_ROOT_DIR}/debug/bin/${LIB_NAME_DEBUG}"
            )
            if(MSVC)
                set_target_properties(${LIB_NAME_WLE_RELEASE} PROPERTIES
                    IMPORTED_IMPLIB_RELEASE "${wxWidgets_ROOT_DIR}/lib/${LIB_NAME_RELEASE}"
                    IMPORTED_IMPLIB_DEBUG "${wxWidgets_ROOT_DIR}/debug/lib/${LIB_NAME_DEBUG}"
                )
            endif()
        endif()

        set_target_properties(${LIB_NAME_WLE_RELEASE} PROPERTIES
            INTERFACE_INCLUDE_DIRECTORIES "${wxWidgets_ROOT_DIR}/include"
            IMPORTED_LINK_INTERFACE_LIBRARIES "${external_LIBS}"
        )
    else()
        list(APPEND _wxWidgets_LIBRARIES ${entry})
    endif()
endforeach()
set(wxWidgets_LIBRARIES ${_wxWidgets_LIBRARIES})

dg0yt · 2022-04-08T05:46:41Z

@jjYBdx4IL I really wanted to test locally, but with your fork removed, this probably won't happen.

talregev · 2022-04-08T05:49:12Z

@jjYBdx4IL Not sure why you close the PR.
Even you take the code inside your project, it can still import to vcpkg and it will be also for other.

jjYBdx4IL · 2022-04-08T08:28:11Z

@jjYBdx4IL I really wanted to test locally, but with your fork removed, this probably won't happen.

I pasted the relevant code here. just insert it after your find_package in your CMakeLists. I won't waste weeks on this crap.

talregev · 2022-04-08T09:27:50Z

@jjYBdx4IL I really wanted to test locally, but with your fork removed, this probably won't happen.

I pasted the relevant code here. just insert it after your find_package in your CMakeLists. I won't waste weeks on this crap.

It doesn't work like this, we don't work like this, open source projects doesn't work like this.

jjYBdx4IL · 2022-04-08T09:39:45Z

It doesn't work like this, we don't work like this, open source projects doesn't work like this.

What currently doesn't work is your CI system.

talregev · 2022-04-08T09:42:01Z

It doesn't work like this, we don't work like this, open source projects doesn't work like this.

What currently doesn't work is your CI system.

I mean the way of work. We want your contribution, and it slow process.
So if you can re-open the PR, update it (if needed) then we can check.
Currently the state of the wxwidgets is not allow me to check it on the ci. with, or without your change.
I am waiting for other fix to be merge, then I can check your PR.

jjYBdx4IL · 2022-04-08T09:45:39Z

I know what you meant. But this process isn't a "process". It's nonsense.

talregev · 2022-04-08T10:18:46Z

It doesn't work like this, we don't work like this, open source projects doesn't work like this.

What currently doesn't work is your CI system.

This is what is not working on my CI:
#24038

jjYBdx4IL added 2 commits April 4, 2022 15:22

* fix wxWidgets_LIBRARIES for windows static builds (add missing comc…

888c290

…tl32 etc.) * add wxbase to list of debug libs

Update version database

a04b677

jjYBdx4IL added 3 commits April 5, 2022 00:02

use @only for file inserts

1b51dcb

update version database

2e7e19e

fix file permissions

839aa05

JonLiu1993 assigned LilyWangLL Apr 6, 2022

LilyWangLL added the category:port-bug The issue is with a library, which is something the port should already support label Apr 6, 2022

LilyWangLL approved these changes Apr 6, 2022

View reviewed changes

LilyWangLL added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Apr 6, 2022

LilyWangLL changed the title ~~fix wxWidgets_LIBRARIES for windows, incl. static builds~~ [wxwidgets] fix wxWidgets_LIBRARIES for windows, incl. static builds Apr 6, 2022

dg0yt reviewed Apr 6, 2022

View reviewed changes

ports/wxwidgets/portfile.cmake Outdated Show resolved Hide resolved

ports/wxwidgets/portfile.cmake Outdated Show resolved Hide resolved

ports/wxwidgets/vcpkg-cmake-wrapper.cmake Outdated Show resolved Hide resolved

LilyWangLL added requires:author-response and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Apr 6, 2022

jjYBdx4IL added 2 commits April 6, 2022 18:46

fix stuff

9c73def

Update version database

084f0ec

github-actions bot requested changes Apr 7, 2022

View reviewed changes

LilyWangLL removed the requires:author-response label Apr 7, 2022

jjYBdx4IL closed this Apr 7, 2022

Conversation

jjYBdx4IL commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does your PR fix?

Which triplets are supported/not supported? Have you updated the CI baseline?

Does your PR follow the maintainer guide?

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

Uh oh!

ghost commented Apr 4, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cenit commented Apr 4, 2022

Uh oh!

jjYBdx4IL commented Apr 4, 2022

Uh oh!

dg0yt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jjYBdx4IL commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

LilyWangLL commented Apr 7, 2022

Uh oh!

jjYBdx4IL commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjYBdx4IL commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dg0yt commented Apr 7, 2022

Uh oh!

dg0yt commented Apr 7, 2022

Uh oh!

jjYBdx4IL commented Apr 7, 2022

Uh oh!

talregev commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dg0yt commented Apr 7, 2022

Uh oh!

jjYBdx4IL commented Apr 7, 2022

Uh oh!

jjYBdx4IL commented Apr 7, 2022

Uh oh!

jjYBdx4IL commented Apr 7, 2022

Uh oh!

dg0yt commented Apr 8, 2022

Uh oh!

talregev commented Apr 8, 2022

Uh oh!

jjYBdx4IL commented Apr 8, 2022

Uh oh!

talregev commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjYBdx4IL commented Apr 8, 2022

Uh oh!

talregev commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjYBdx4IL commented Apr 8, 2022

Uh oh!

talregev commented Apr 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jjYBdx4IL commented Apr 4, 2022 •

edited

Loading

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

ghost commented Apr 4, 2022 •

edited by ghost

Loading

dg0yt left a comment •

edited

Loading

jjYBdx4IL commented Apr 6, 2022 •

edited

Loading

jjYBdx4IL commented Apr 7, 2022 •

edited

Loading

jjYBdx4IL commented Apr 7, 2022 •

edited

Loading

talregev commented Apr 7, 2022 •

edited

Loading

talregev commented Apr 8, 2022 •

edited

Loading

talregev commented Apr 8, 2022 •

edited

Loading