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] Update and fix #25572

Merged
merged 18 commits into from
Jul 11, 2022
Merged

[wxwidgets] Update and fix #25572

merged 18 commits into from
Jul 11, 2022

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Jul 5, 2022

github-actions[bot]
github-actions bot previously approved these changes Jul 5, 2022
@AenBleidd
Copy link
Contributor

I don't agree with this PR. Please read my comment: #25437 (comment)

@JackBoosY JackBoosY self-assigned this Jul 5, 2022
@JackBoosY JackBoosY added category:port-bug The issue is with a library, which is something the port should already support category:port-update The issue is with a library, which is requesting update new revision labels Jul 5, 2022
@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 5, 2022

I don't agree with this PR. Please read my comment: #25437 (comment)

I assume you don't refer to the PR in general, but to the wxDEBUG_LEVEL aspect. For this aspect, my PR simply reverts the default (upstream) wxwidgets configuration as it is an immediate solution which unbreaks user projects. This is what you suggest:

it was decided that we should revert my change

This PR doesn't mean to provide a more advanced long-term solution "to control debug asserts in a more flexible way".
Which might be the outcome of #25551, if done right.

@AenBleidd
Copy link
Contributor

@dg0yt, so maybe this PR and #25551 should not be merged separately then? Otherwise this PR will create regression if merged first.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 5, 2022

Otherwise this PR will create regression if merged first.

I don't see how this PR creates a regression. It adds symbols, but it doesn't limit downstream usage. This is different from #25240 which removed things, causing regressions downstream. That's why this PR is a fix, even if it some might dislike the wxwidget's default debug level.

@AenBleidd
Copy link
Contributor

@dg0yt, it will lead to this error again: BOINC/boinc#4806
When application requests complete removal of debug asserts, they will be still in wxwidgets and still enabled. Please see our discussion in the ticket I mentioned

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 5, 2022

it will lead to this error again: BOINC/boinc#4806 When application requests complete removal of debug asserts, they will be still in wxwidgets and still enabled. Please see our discussion in the ticket I mentioned

Possibly yes, but this may just indicate that you didn't find the right solution.
AFAIU boinc aready sets NDEBUG, but I don't see that it ensures calling wxDISABLE_DEBUG_SUPPORT for release builds.
(No search hits on GH for wxDISABLE_DEBUG_SUPPORT or wxIMPLEMENT_APP, but this is what the docs says:
https://docs.wxwidgets.org/trunk/overview_debugging.html#overview_debugging_config)

And include/wx/debug.h says:

0:  No assertion macros at all, this should only be used when optimizing
        for resource-constrained systems (typically embedded ones)

https://github.com/wxWidgets/wxWidgets/blob/7ad1bffa875f7a38db58b6069ea3ff0c49c211d2/include/wx/debug.h#L32-L33

@dg0yt dg0yt marked this pull request as draft July 5, 2022 17:28
@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 5, 2022

For native mingw builds (on windows), I switched to the MSVC layout, so that the find module can operatore in win32 mode, not needing sh for wx-config. This reduces the amount of patching and wrapping.
Downside: there is no wx-config for mingw on windows. Mingw on linux is different.

github-actions[bot]
github-actions bot previously approved these changes Jul 5, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 5, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 5, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 6, 2022
@dg0yt dg0yt marked this pull request as ready for review July 6, 2022 05:13
@dg0yt dg0yt marked this pull request as draft July 6, 2022 06:38
@JackBoosY
Copy link
Contributor

Ready for review?

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • scripts/test_ports/vcpkg-ci-wxwidgets/vcpkg.json

Valid values for the license field can be found in the documentation

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 6, 2022

Configuration without debug support now testable with:

./vcpkg install --overlay-ports scripts/test_ports wxwidgets[core] vcpkg-ci-wxwidgets

@dg0yt dg0yt marked this pull request as ready for review July 6, 2022 07:22
@talregev
Copy link
Contributor

talregev commented Jul 8, 2022

@JackBoosY Can you review it?

@marekr
Copy link
Contributor

marekr commented Jul 8, 2022

wx 3.2 is now available fyi, basically 3.1.7 with a minor bug fixes and documentation cleanup.

There's also now a cmake config file generated rather than needing findwxwidgets

@talregev
Copy link
Contributor

talregev commented Jul 8, 2022

I think we need to do 3.1.7 then go to 3.2.0.

@marekr
Copy link
Contributor

marekr commented Jul 8, 2022

I think we need to do 3.1.7 then go to 3.2.0.

I would argue it's not worth it. 3.2.0 is a patch release on 3.1.7 and just bug fixes. wx doesn't follow semver.

@talregev
Copy link
Contributor

talregev commented Jul 8, 2022

For 3.2.0 I think they will follow semver:

wxWidgets 3.2.0 is the first release in the 3.2 stable branch. This means that it will remain ABI-compatible with all the 3.2.x releases in the future, i.e. the applications using it will continue to work with the future releases in this series even without recompiling.

Also if it almost the same, then we can do 2 PRs. one for 3.1.7 then go to 3.2.0. Then users for vcpkg can choose versions in manifests mode. More versions to choose from, in a price of 2 commit and a couple of days delay.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 8, 2022

This PR is 3.1.7, and it is ready. Merge as is unless there is a real issue, and do 3.2.0 after that.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Jul 11, 2022
@vicroms vicroms merged commit 88b3aed into microsoft:master Jul 11, 2022
@dg0yt dg0yt deleted the wxwidgets branch July 12, 2022 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
6 participants