Set default build compatibility in CMake builds to 3.2 (PR #22715)
PB
Remove "2.8" and "3.1" from wxBUILD_COMPATIBILITY options.
Add "3.2" to wxBUILD_COMPATIBILITY options and set it as its default value.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/22715
Commit Summary
- cc82cc3 Set default build compatibility in CMake builds to 3.2
File Changes
(1 file)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/22715.patch
- https://github.com/wxWidgets/wxWidgets/pull/22715.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
I have realized that this change is not (entirely) correct and why there actually was "3.1" compatibility option.
The change I proposed would not allow to turn off any compatibility, the mapping from CMake option wxBUILD_COMPATIBILITY to code define WXWIN_COMPATIBILITY_x_y needs a "fake" compatibility option (e.g., "3.3" atm) for that, as shown here:
https://github.com/wxWidgets/wxWidgets/blob/d0e4aa761cf716f17875a648dff5effd55a05c6a/build/cmake/init.cmake#L97-L102
BTW, when the docs for wxBUILD_COMPATIBILITY are changed, it needs to reflect in its description in the documentation, which I have not done (yet).
No really sure what to do, I guess following the established practice and adding "3.3" option is the only choice even if no such API compatibility exists.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I think setting wxBUILD_COMPATIBILITY == 3.3 does make sense, if you squint enough, but I believe it would be even better to use some special and more clear value for it meaning that no compatibility with previous versions is being requested, e.g. wxBUILD_COMPATIBILITY=only-latest or something like this.
But, again, I'm fine with adding 3.3 compatibility too and this is the simplest solution, so if you could please update the PR to do it (and update the docs too) it would be great. TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
@PBfordev pushed 1 commit.
- 82d7dfa Set default build compatibility in CMake builds to 3.2
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
I chose out a bit different approach than before but perhaps there was a reason to do it the former way and @MaartenBent knows why...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz approved this pull request.
Thanks, this looks better for me too, so I'll merge it if Maarten has no objections.
In docs/doxygen/overviews/cmake.md:
> @@ -69,7 +69,7 @@ wxBUILD_TESTS | STRING | OFF | CONSOLE_ONLY, ALL or OFF wxBUILD_SAMPLES | STRING | OFF | SOME, ALL or OFF wxBUILD_DEMOS | BOOL | OFF | Build demo applications wxUSE_GUI | BOOL | ON | Build the UI libraries -wxBUILD_COMPATIBILITY | STRING | 3.0 | 2.8, 3.0 or 3.1 API compatibility +wxBUILD_COMPATIBILITY | STRING | 3.2 | API compatibility with 3.0, 3.2 or none
If you don't mind, I'd like to change it like this when applying:
⬇️ Suggested change-wxBUILD_COMPATIBILITY | STRING | 3.2 | API compatibility with 3.0, 3.2 or none
+wxBUILD_COMPATIBILITY | STRING | 3.2 | Enable API compatibility with 3.0, 3.2 or neither ("none").
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
@PBfordev commented on this pull request.
In docs/doxygen/overviews/cmake.md:
> @@ -69,7 +69,7 @@ wxBUILD_TESTS | STRING | OFF | CONSOLE_ONLY, ALL or OFF wxBUILD_SAMPLES | STRING | OFF | SOME, ALL or OFF wxBUILD_DEMOS | BOOL | OFF | Build demo applications wxUSE_GUI | BOOL | ON | Build the UI libraries -wxBUILD_COMPATIBILITY | STRING | 3.0 | 2.8, 3.0 or 3.1 API compatibility +wxBUILD_COMPATIBILITY | STRING | 3.2 | API compatibility with 3.0, 3.2 or none
@vadz, I agree with the suggestion.
Looking at the suggestion, I have realized I made a mistake: Other string options are always in upper case, so "none" should be changed to "NONE" everywhere.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
@PBfordev pushed 1 commit.
- bbc9add Yet another attempt at build compatibility in CMake
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
I have pushed yet another version, this time allowing to set 3.0 and 3.2 compatibility independently.
BTW, I have noticed that the comment in setup.h for WXWIN_COMPATIBILITY_3_0 is probably incorrect:
https://github.com/wxWidgets/wxWidgets/blob/8c9b09dfa95a5ea8d4d1df2da01183b2dc1cb3ce/include/wx/msw/setup.h#L29-L37
The comment is virtually identical to that for WXWIN_COMPATIBILITY_3_2. However, not only is the WXWIN_COMPATIBILITY_3_0 default value 0 and not 1 ; I also believe that the deprecated API is going to be removed in the next major version instead of the one after it?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Sorry, I don't think the last version is correct. It shouldn't be possible to enable 3.0 compatibility without enabling 3.2 compatibility, this doesn't make much sense in theory and is definitely not useful in practice. We really have just 3 levels:
- Compatibility with both 3.0 and 3.2 enabled.
- Compatibility only with 3.2 enabled (default in 3.3).
- Compatibility entirely disabled.
Also, we really remove the deprecated functionality in the version after the one in which it was deprecated, i.e. the API life cycle is: supported -> supported, but deprecated, but still enabled by default -> still supported, but not enabled by default any longer -> not supported, where each arrow corresponds to a minor version change. And yes, it means that we still support 2.8 compatibility in 3.2 (although you need to explicitly enable it).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
Vadim, I do realize that for obvious reasons it would be beyond silly for me to attempt to argue with you about anything wxWidgets-related. Be that as it may...
Sorry, I don't think the last version is correct. It shouldn't be possible to enable 3.0 compatibility without enabling 3.2 compatibility
I did not know this (I could not find it documented anywhere), since setup.h allows to set the two independently and the #if WXWIN_COMPATIBILITY_3_x guards in the code are also specific only for version x . Either way, my previous attempt must have also been wrong, as setting compatibility to 3.2 did not set 3.0 compatibility either. But it is the same for the current CMake version and it was analogical for 3.1.x (i.e., setting 3.0 compatibility ON did not set ON 2.8 compatibility)? I have no idea how it works with configure.
Also, we really remove the deprecated functionality in the version after the one in which it was deprecated, i.e. the API life cycle is: supported -> supported, but deprecated, but still enabled by default -> still supported, but not enabled by default any longer -> not supported, where each arrow corresponds to a minor version change.
But this supports my "WXWIN_COMPATIBILITY_3_0 comment is wrong" argument as the 3.0-compatible APIs (i.e., "still supported, but not enabled by default any longer") should be removed in 3.4 and not 3.6? Either way, the incorrect default value in the comment aside, IMO it would not make sense to have the same comment including the version indicated for API removal for both WXWIN_COMPATIBILITY_3_0 and WXWIN_COMPATIBILITY_3_2.
To conclude: Please confirm that I really should I push the version where setting wxBUILD_COMPATIBILITY to 3.2 sets ON not only WXWIN_COMPATIBILITY_3_2 but also WXWIN_COMPATIBILITY_3_0 .
BTW, #19151 can probably be closed now (see eb1242c).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Vadim, I do realize that for obvious reasons it would be beyond silly for me to attempt to argue with you about anything wxWidgets-related.
You should definitely argue with me when you think I'm wrong, which happens with depressing (at least for me) regularity, if only because I often misread what people write in a hurry! But I still think that I'm right in this particular situation :-)
Sorry, I don't think the last version is correct. It shouldn't be possible to enable 3.0 compatibility without enabling 3.2 compatibility
I did not know this (I could not find it documented anywhere),
It probably should be, but to me it makes sense: you enable compatibility with the N-2 version because you have an old application which was written using it and you want to still compile it. You disable compatibility with the N-1 version because you want to ensure that you don't use any deprecated APIs. Why would you ever want to enable N-2 compatibility but disable compatibility with N-1?
since setup.h allows to set the two independently
It doesn't, actually, see https://github.com/wxWidgets/wxWidgets/blob/8c9b09dfa95a5ea8d4d1df2da01183b2dc1cb3ce/include/wx/chkconf.h#L1331-L1340
Either way, my previous attempt must have also been wrong, as setting compatibility to 3.2 did not set 3.0 compatibility either.
This is correct, it works the other way round.
But this supports my "
WXWIN_COMPATIBILITY_3_0comment is wrong" argument as the 3.0-compatible APIs (i.e., "still supported, but not enabled by default any longer") should be removed in 3.4 and not 3.6?
No, 3.0 API deprecated in 3.2 is disabled (but still present) in 3.4 and only removed in 3.6.
To conclude: Please confirm that I really should I push the version where setting
wxBUILD_COMPATIBILITYto3.2sets ON not onlyWXWIN_COMPATIBILITY_3_2but alsoWXWIN_COMPATIBILITY_3_0.
No, sorry, I must have missed that it worked like this, but it was wrong. It's setting compatibility to 3.0 that should enable 3.2 compatibility too, not the other way round.
Indeed, thanks for the reminder!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
Vadim, sorry for misunderstanding how the API backward compatibility works and thanks for the explanation.
I have pushed another attempt at this.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
@PBfordev pushed 1 commit.
- 4fb6da1 !fixup
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
This (again :-) looks good to me, thanks!
So let me just ping @MaartenBent for one last time and I'll merge it if there are no objections.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
> @@ -94,10 +94,10 @@ elseif(("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU") OR ("${CMAKE_CXX_COMPILER_ID}
endif()
endif()
-if(wxBUILD_COMPATIBILITY VERSION_LESS 3.2)
+if(wxBUILD_COMPATIBILITY STREQUAL "3.0")
If users have wxBUILD_COMPATIBILITY 2.8 in their current configuration, this will now result in them having no compatibility at all.
I think it is better to move them to 3.0 by keeping the VERSION_LESS checks.
I'm not sure if you need a check for NONE or if you can rely on the version checks to fail.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
@PBfordev pushed 1 commit.
- 71d0ace !fixup
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
@PBfordev commented on this pull request.
> @@ -94,10 +94,10 @@ elseif(("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU") OR ("${CMAKE_CXX_COMPILER_ID}
endif()
endif()
-if(wxBUILD_COMPATIBILITY VERSION_LESS 3.2)
+if(wxBUILD_COMPATIBILITY STREQUAL "3.0")
I have pushed yet another commit, with changes based on Maarten's comment.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks, looks perfect to me, will merge soon.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.