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] Add feature wxdebug-level #25551

Closed
wants to merge 2 commits into from

Conversation

Adela0814
Copy link
Contributor

Fix #25437

Changed the option to port feature.

@Adela0814 Adela0814 added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Jul 4, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 4, 2022
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

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

IMO the current state of this PR is invalid:
The activation of feature wxdebug-level removes API. This is not allowed in vcpkg: Downstream ports could no longer express their dependency.
If anything, the default debug level (1) can be tied to a default feature.
And the debug level 0 should be burned into the headers in a way that downstream ports follow the wxwidgets ports configuration.

ports/wxwidgets/portfile.cmake Outdated Show resolved Hide resolved
ports/wxwidgets/vcpkg.json Outdated Show resolved Hide resolved
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

OPTIONS_RELEASE
-DwxBUILD_DEBUG_LEVEL=0
OPTIONS
${FEATURE_OPTIONS}
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume, this still should be an OPTIONS_RELEASE because in general it's nice to have wxBUILD_DEBUG_LEVEL=1 when building in Debug mode

Comment on lines +16 to +17
OPTIONS
${FEATURE_OPTIONS}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
OPTIONS
${FEATURE_OPTIONS}
OPTIONS_RELEASE
-DwxBUILD_DEBUG_LEVEL=0
OPTIONS_DEBUG
-DwxBUILD_DEBUG_LEVEL=1

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid, this will not solve the original issue described here: #25437

@Adela0814
Copy link
Contributor Author

Duplicate of #25572

@Adela0814 Adela0814 marked this as a duplicate of #25572 Jul 5, 2022
@Adela0814 Adela0814 closed this Jul 5, 2022
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

It is still the wrong approach. If one downstream port requests wxwidgets[wxdebug-level-0], it will remove behaviours for other downstream ports.

"Features must be treated as additive functionality."
Wxwidget's debug support is a set of behaviours which are enabled by the recommended default level 1.
So the natural representation for level 1 is a default feature in vcpkg.
Cf. "Default features should enable behaviors, not APIs"

@AenBleidd
Copy link
Contributor

@dg0yt, since this is closed, do you have an idea how to implement this feature in a correct way?

@dg0yt
Copy link
Contributor

dg0yt commented Jul 5, 2022

since this is closed, do you have an idea how to implement this feature in a correct way?

I outlined this already twice in this PR: There must be default feature to control (i.e. enable )wxwidgets default debugging support.
In that way, you get default behaviour by default, and can opt out on demand.

@Adela0814 Adela0814 deleted the Dev/Mengna/wxwidgets branch January 16, 2023 06:23
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 info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[wxWidgets] wxDEBUG_LEVEL should not be set to 0
6 participants