[wxwidgets] make wxUSE_STL an optional triplet feature disabled by default by Be-ing · Pull Request #19274 · microsoft/vcpkg

Be-ing · 2021-08-01T02:49:47Z

wxUSE_STL defaults to OFF so that is how it is in Linux
distribution packages. Downstream projects developed with
wxUSE_STL=ON are not necessarily compatible with wxUSE_STL=OFF
without modification. So, by default, go with wxWidgets' default
for compatibility with downstream codebases. vcpkg users who need
wxUSE_STL=ON can still do so by simply opting into the new 'stl'
feature of this port.

https://forums.wxwidgets.org/viewtopic.php?p=165208

Describe the pull request

What does your PR fix?

Fixes downstream applications not building with wxwidgets port
Which triplets are supported/not supported? Have you updated the CI baseline?

all
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

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 261c458af6e3eed5d099144aff95d2b5035f656b -- versions
./vcpkg x-add-version --all

Diff

diff --git a/versions/baseline.json b/versions/baseline.json
index 7e64191..b042cca 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -6738,7 +6738,7 @@
     },
     "wxwidgets": {
       "baseline": "3.1.5",
-      "port-version": 2
+      "port-version": 3
     },
     "x-plane": {
       "baseline": "3.0.3",
diff --git a/versions/w-/wxwidgets.json b/versions/w-/wxwidgets.json
index 72eaf13..38622b9 100644
--- a/versions/w-/wxwidgets.json
+++ b/versions/w-/wxwidgets.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "3561ee078b11facb11fbd807f03093262cd52f19",
+      "version-semver": "3.1.5",
+      "port-version": 3
+    },
     {
       "git-tree": "6fa230bffdee1e7d700570c31e6f08367460c0c9",
       "version-semver": "3.1.5",

PhoebeHui

LGTM

dg0yt · 2021-08-02T06:40:08Z

I would like to ask for clarification whether this feature is additive and compatible with binary caching:

How does it affect ABI?
How does it affect the library's backward compatibility?
Basically, it Port B requires wxwidgets[stl], may it break Port A which was built agains wxwidgets[core]?

Be-ing · 2021-08-02T12:17:43Z

wxUSE_STL=ON and wxUSE_STL=OFF are not source compatible so I don't imagine they would be ABI compatible.

Basically, it Port B requires wxwidgets[stl], may it break Port A which was built agains wxwidgets[core]?

I suppose so.

dg0yt · 2021-08-02T13:35:17Z

wxUSE_STL=ON and wxUSE_STL=OFF are not source compatible so I don't imagine they would be ABI compatible.

Basically, it Port B requires wxwidgets[stl], may it break Port A which was built agains wxwidgets[core]?

I suppose so.

This is the list of "Do not" for features:
https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#features

I get the impression that the proposed feature is in conflict with at least one item.

Be-ing · 2021-08-02T13:38:04Z

The only alternatives are:

Keep this port as-is which is unusable for lots of wxWidgets applications.
Provide no option and only use wxUSE_STL=OFF which would make the upstream port unusable if anyone was previously depending on wxUSE_STL=ON.

Be-ing · 2021-08-02T13:41:47Z

https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#do-not-use-features-to-control-alternatives-in-published-interfaces lists some exceptions made for backwards compatibility. I think this case should have an exception made for backwards compatibility.

dg0yt · 2021-08-02T16:29:17Z

I think this case should have an exception made for backwards compatibility.

Don't misunderstand backwards compatibility and the existing exceptions: These ports already had a broken feature list. The feature were left for backwards compatibility.

The only alternatives are:

Keep this port as-is which is unusable for lots of wxWidgets applications.

Provide no option and only use wxUSE_STL=OFF which would make the upstream port unusable if anyone was previously depending on wxUSE_STL=ON.

There is more:

Let the user use overlay ports to change the default.
Use another variable in the portfile, and let users override this variable in custom triplets if needed.

Be-ing · 2021-08-02T16:32:49Z

Let the user use overlay ports to change the default.

I don't think this is a good idea because the port currently does not work with downstream codebases that were developed with wxWidgets' default of wxUSE_STL=OFF.

Use another variable in the portfile, and let users override this variable in custom triplets if needed.

This would be quite cumbersome. I don't know why that would be better than a port feature.

dg0yt · 2021-08-02T16:49:33Z

Use another variable in the portfile, and let users override this variable in custom triplets if needed.

This would be quite cumbersome. I don't know why that would be better than a port feature.

It creates an entire consistent universe of compatible binaries bound to that particular triplet.

With features, this is not possible: The binaries created from wxwidgets[stl] are incompatible with ports depending on wxwidgets[core] as far as we are told. This is not additive.

Be-ing · 2021-08-02T16:52:41Z

I would be annoyed if I had to use a custom triplet just to use wxWidgets with its upstream defaults. But if that's what the vcpkg maintainers decide, it should be documented.

strega-nil-ms · 2021-08-05T23:07:08Z

Do you have a repro? I'm trying to figure out why this is necessary, except for the wxUSE_STL_BASED_WXSTRING stuff (which we could fix by defining that).

Be-ing · 2021-08-06T00:10:59Z

I am working with this branch of Tenacity: https://github.com/tenacityteam/tenacity/pull/228 This branch is still very much a work-in-progress and I am force pushing frequently.

When I tried to build that branch with the wxwidgets port in wxWidgets/wxWidgets#2402 which uses wxUSE_STL=ON, the build failed with these errors. When I build wxwidgets manually without vcpkg using the default wxUSE_STL=OFF, I am able to build that branch. When I rebuilt wxwidgets manually with wxUSE_STL=ON, I reproduced the compile errors.

JackBoosY · 2021-08-06T02:25:18Z

I am working with this branch of Tenacity: tenacityteam/tenacity#228 This branch is still very much a work-in-progress and I am force pushing frequently.

When I tried to build that branch with the wxwidgets port in wxWidgets/wxWidgets#2402 which uses wxUSE_STL=ON, the build failed with these errors. When I build wxwidgets manually without vcpkg using the default wxUSE_STL=OFF, I am able to build that branch. When I rebuilt wxwidgets manually with wxUSE_STL=ON, I reproduced the compile errors.

Those failures are due to the upstream doesn't handle the library path. See wxWidgets/wxWidgets#2402.

talregev · 2021-08-06T04:53:47Z

@JackBoosY Will you help us to continue the PR that solve the problems?

Be-ing · 2021-08-06T16:05:08Z

Those failures are due to the upstream doesn't handle the library path. See wxWidgets/wxWidgets#2402.

Yes the first error I posted in https://github.com/tenacityteam/tenacity/pull/228#issuecomment-880113285 was due to that upstream bug. But then I ran into the compile errors because of wxUSE_STL=ON.

Be-ing · 2021-08-06T16:07:28Z

https://docs.wxwidgets.org/3.0/classwx_string.html has this note:

If you built wxWidgets with wxUSE_STL set to 1, the implicit conversions to both narrow and wide C strings are disabled and replaced with implicit conversions to std::string and std::wstring.

I think this is the source of the compile error I encountered with wxUSE_STL=ON.

strega-nil-ms · 2021-08-06T19:56:54Z

@Be-ing could you try instead with wxUSE_STL=1, and wxUSE_STL_BASED_WXSTRING=0? I think that that's where the real issue lies.

Be-ing · 2021-08-06T20:01:26Z

That's not an available CMake variable:

CMake Warning:
  Manually-specified variables were not used by the project:

    wxUSE_STL_BASED_WXSTRING

strega-nil-ms · 2021-08-06T20:40:19Z

urggh... okay, I think the best thing is to default to -DwxUSE_STL=OFF then, and allow for turning it back on.

Be-ing · 2021-08-06T20:52:14Z

So merge this PR?

strega-nil-ms · 2021-08-09T16:57:44Z

@Be-ing no, it shouldn't be a feature. Could you turn it off by default, and make turning it back on a triplet option?

Be-ing · 2021-08-09T21:25:37Z

Okay, that sounds like a good solution. Is there documentation or an example how to do that? Just set the wxUSE_STL CMake option based on a CMake variable? Then users can choose to set that CMake variable in a custom triplet?

Be-ing · 2021-08-09T22:15:57Z

Where would I document that the triplet option exists?

dg0yt · 2021-08-10T06:38:04Z

Where would I document that the triplet option exists?

Good question. Documentation exists for global triplet variables, but not for per-port variables. How about adding a sentence to the port's description field? In this way, it would be visible in online package search. E.g.

To enable 'wxUSE_STL', set 'VCPKG_WXWIDGETS_USE_STL' to 'ON' in a custom triplet file.

ports/wxwidgets/portfile.cmake

ports/wxwidgets/vcpkg.json

wxUSE_STL defaults to OFF so that is how it is in Linux distribution packages. Downstream projects developed with wxUSE_STL=ON are not necessarily compatible with wxUSE_STL=OFF without modification. So, by default, go with wxWidgets' default for compatibility with downstream codebases. vcpkg users who need wxUSE_STL=ON can still do so by simply opting into the new 'stl' feature of this port. https://forums.wxwidgets.org/viewtopic.php?p=165208 Signed-off-by: Be <be@mixxx.org>

Be-ing · 2021-08-19T14:34:23Z

This has been approved by 3 reviewers. Can it be merged?

Be-ing · 2021-08-19T16:12:41Z

Thanks for reviewing. I am glad we found a solution that works with vcpkg's policies. Now, onto #17111...

talregev · 2021-08-19T16:22:27Z

@Be-ing @JackBoosY

Now, onto #17111...

I so agree ...

Be-ing mentioned this pull request Aug 1, 2021

Add CMake modules and build without vendored dependencies using vcpkg tenacityteam/tenacity-legacy#228

Merged

5 tasks

github-actions bot requested changes Aug 1, 2021

View reviewed changes

Be-ing force-pushed the wxwidgets_stl branch from 6fca4d8 to 57a60e8 Compare August 1, 2021 02:54

github-actions bot approved these changes Aug 1, 2021

View reviewed changes

Be-ing mentioned this pull request Aug 1, 2021

[wxwidgets] Fix usage, add cmake options #17111

Closed

JonLiu1993 assigned PhoebeHui Aug 2, 2021

PhoebeHui added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Aug 2, 2021

PhoebeHui requested a review from JackBoosY August 2, 2021 06:03

PhoebeHui approved these changes Aug 2, 2021

View reviewed changes

PhoebeHui added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Aug 2, 2021

JackBoosY approved these changes Aug 2, 2021

View reviewed changes

PhoebeHui added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Aug 3, 2021

Be-ing force-pushed the wxwidgets_stl branch from 57a60e8 to 707ba74 Compare August 11, 2021 15:18

Be-ing requested a review from JackBoosY August 11, 2021 15:20

Be-ing changed the title ~~[wxwidgets] make wxUSE_STL an optional feature disabled by default~~ [wxwidgets] make wxUSE_STL an optional triplet feature disabled by default Aug 11, 2021

github-actions bot approved these changes Aug 11, 2021

View reviewed changes

Be-ing force-pushed the wxwidgets_stl branch from 707ba74 to 8d2a55f Compare August 11, 2021 15:21

github-actions bot approved these changes Aug 11, 2021

View reviewed changes

talregev suggested changes Aug 11, 2021

View reviewed changes

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

ports/wxwidgets/vcpkg.json Outdated Show resolved Hide resolved

strega-nil-ms approved these changes Aug 11, 2021

View reviewed changes

talregev approved these changes Aug 11, 2021

View reviewed changes

Be-ing force-pushed the wxwidgets_stl branch from 8d2a55f to 7185333 Compare August 11, 2021 17:32

github-actions bot approved these changes Aug 11, 2021

View reviewed changes

PhoebeHui removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Aug 12, 2021

JackBoosY approved these changes Aug 12, 2021

View reviewed changes

strega-nil-ms merged commit 996baef into microsoft:master Aug 19, 2021

Be-ing deleted the wxwidgets_stl branch August 19, 2021 16:11

Karandra mentioned this pull request Dec 8, 2021

[wxwidgets] Add 'WXWIDGETS_USE_STD_CONTAINERS' option #21841

Merged

Conversation

Be-ing commented Aug 1, 2021

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!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

PhoebeHui left a comment

Choose a reason for hiding this comment

Uh oh!

dg0yt commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Be-ing commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dg0yt commented Aug 2, 2021

Uh oh!

Be-ing commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Be-ing commented Aug 2, 2021

Uh oh!

dg0yt commented Aug 2, 2021

Uh oh!

Be-ing commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dg0yt commented Aug 2, 2021

Uh oh!

Be-ing commented Aug 2, 2021

Uh oh!

strega-nil-ms commented Aug 5, 2021

Uh oh!

Be-ing commented Aug 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JackBoosY commented Aug 6, 2021

Uh oh!

talregev commented Aug 6, 2021

Uh oh!

Be-ing commented Aug 6, 2021

Uh oh!

Be-ing commented Aug 6, 2021

Uh oh!

strega-nil-ms commented Aug 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Be-ing commented Aug 6, 2021

Uh oh!

strega-nil-ms commented Aug 6, 2021

Uh oh!

Be-ing commented Aug 6, 2021

Uh oh!

strega-nil-ms commented Aug 9, 2021

Uh oh!

Be-ing commented Aug 9, 2021

Uh oh!

Be-ing commented Aug 9, 2021

Uh oh!

dg0yt commented Aug 10, 2021

Uh oh!

Uh oh!

Uh oh!

Be-ing commented Aug 19, 2021

Uh oh!

Be-ing commented Aug 19, 2021

Uh oh!

talregev commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

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

dg0yt commented Aug 2, 2021 •

edited

Loading

Be-ing commented Aug 2, 2021 •

edited

Loading

Be-ing commented Aug 2, 2021 •

edited

Loading

Be-ing commented Aug 2, 2021 •

edited

Loading

Be-ing commented Aug 6, 2021 •

edited

Loading

strega-nil-ms commented Aug 6, 2021 •

edited

Loading

talregev commented Aug 19, 2021 •

edited

Loading