-
Notifications
You must be signed in to change notification settings - Fork 7.3k
[vcpkg-make,ncurses,python3] Filter default options, disables curses in python #49187
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
base: master
Are you sure you want to change the base?
Conversation
It was always enabled. When it needs to be disabled, the effect can be achieved with DEFAULT_OPTIONS_EXCLUDE.
| +_curses | ||
| +_curses_panel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes back to #28273, but it simply had no effect at the end of Setup:
maksetup: '_curses' was handled by previous rule.
maksetup: '_curses_panel' was handled by previous rule.
In theory, it would be possible to customize Setup.local ... but it would have to be done in the build directory, and in my tests make altinstall simply overwrote it with an empty placeholder.
#28273 was for osx problems. In theory it would be possible to patch python to use vcpkg ncurses. But it seems that it would need more work to avoid system provided paths even on non-osx.
And with vcpkg readline, we pull in a binary dependency on vcpkg ncurses.
Conclusion: KISS. Disable _curses and _curses_panel, reliably.
| py_cv_module__curses=n/a | ||
| py_cv_module__curses_panel=n/a | ||
| py_cv_module__tkinter=n/a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n/a is what is used by python configure when it disables extensions for some platforms.
ports/python3/portfile.cmake
Outdated
| vcpkg_make_configure( | ||
| SOURCE_PATH "${SOURCE_PATH}" | ||
| AUTORECONF | ||
| DEFAULT_OPTIONS_EXCLUDE "^--disable-silent-rules|^--enable-static" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strage coincidence, drive-by fix. Touching python3 because it stumbled over ncurses for this PR. And I found python3 complaining:
configure: WARNING: unrecognized options: --disable-silent-rules, --enable-static
configure: WARNING: unrecognized options: --disable-silent-rules, --enable-static
So using the new option added by this PR...
| z_vcpkg_make_default_path_and_configure_options(opts CONFIG "${configup}" | ||
| EXCLUDE_FILTER "${arg_DEFAULT_OPTIONS_EXCLUDE}" | ||
| ) | ||
| elseif(arg_DEFAULT_OPTIONS_EXCLUDE OR arg_DEFAULT_OPTIONS_INCLUDE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see DEFAULT_OPTIONS_INCLUDE does not exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the condition that actually must be forbidden is arg_DISABLE_DEFAULT_OPTIONS + arg_DEFAULT_OPTIONS_EXCLUDE and it should probably be done near the top of the function where all the other parameter validation is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First I added add DEFAULT_OPTIONS_INCLUDE because it the helper function had it, but then I removed it for keeping the interface simple. Now this seems to be left over.
| if(NOT arg_AUTOMAKE) | ||
| vcpkg_list(APPEND opts --disable-silent-rules --verbose) | ||
| endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: There was only one caller of z_vcpkg_make_default_path_and_configure_options and it passed AUTOMAKE so this block was dead.
No change requested.
| if(NOT arg_DISABLE_DEFAULT_OPTIONS) | ||
| z_vcpkg_make_default_path_and_configure_options(opts AUTOMAKE CONFIG "${configup}") | ||
| z_vcpkg_make_default_path_and_configure_options(opts CONFIG "${configup}" | ||
| EXCLUDE_FILTER "${arg_DEFAULT_OPTIONS_EXCLUDE}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you speak to having an exclude filter rather than an include filter? It seems like the port documenting the switches that a particular configure script does support is a bit more robust than trying to exclude things it does not but I am NOT an autotools expert...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(To be clear, I'm asking for a record here in code review, I'm not asking you to change the code, I just want to understand better)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, we start with default options, not with no options. And then we add was is needed for specific features/dependencies, and now we can also remove what raises warnings or errors.
Note that for ncurses, the problems really came from starting with no options. The default libdir is right in CI (lib) but not for some distros (lib64). The default options literally point at this fact. But this not something you would discover from configure --help.
| endif() | ||
|
|
||
| set(library_path_flag "${VCPKG_DETECTED_CMAKE_LIBRARY_PATH_FLAG}") | ||
| string(REPLACE " " "\\ " current_installed_dir_escaped "${CURRENT_INSTALLED_DIR}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block was just moved up so I'm not going to discuss/investigate whether this transformation is valid. No change requested.
| CONFIGURE_ENVIRONMENT_VARIABLES CFLAGS | ||
| DETERMINE_BUILD_TRIPLET | ||
| NO_ADDITIONAL_PATHS | ||
| DEFAULT_OPTIONS_EXCLUDE "^--docdir" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this interface truly be a regex or would this being a list of argument names that aren't supported make more sense? (I'm worried about future us forgetting the ^)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without regex, this would also need to learn the split between option and value (--docdir is for --docdir=share/<port>).
The single regex makes it a little bit more distinct with regard to the actual OPTIONS.
But I have no strong opinion here. The current form has a simple implementation, and its use should not be very frequent.
BillyONeal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request-changes only to remove the DEFAULT_OPTIONS_INCLUDE dead code.
This PR enables excluding of default configuration options via a regular expression and uses it to fix ncurses.
vcpkg_configure_make/vcpkg_make_configureare being used not only for actual autotools builds but also for packages which are loosely modeled after after autotool'sconfigure. Some packages, such as ncurses, fail to configure with unsupported options (e.g.--docdir). OTOH they need many of the default options for robust vcpkg integration regardless of host system (e.g.--libdir). The newDEFAULT_OPTIONS_EXCLUDE <regex>option enables precise removal of unsupported options. This is much simpler and more robust than duplicating desired debug/release options for affected ports.Fixes #49106.
Python is a package which issues warnings for default options, so it benefits from the new ``DEFAULT_OPTIONS_EXCLUDE`. But the actual fix in this PR is to reliably disable direct use curses. (It wouldn't correctly use vcpkg ncurses.) This python change might fix:
#48227 (linux), #46980 (osx)
And I finally noticed again
LDFLAGSstarting with-Xlinker -Xlinker -Xlinker-libpath:<vcpkg...>. Now doing the addition of-libpath:...early, and letting the existing proper transformation prepend-Xlinker...