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

waf: allow any custom build option to be specified in waf configure #28039

Merged
merged 4 commits into from
Sep 11, 2024

Conversation

tridge
Copy link
Contributor

@tridge tridge commented Sep 8, 2024

this makes it easy to configure with any option from build_options.py, with tab completion

Note that the build_options.py table takes an optional enable-XXX label to allow us to replace some of the existing options (eg. --enable-ppp and --disable-ekf3)

we end up with options like this:

    --enable-ppp        Enable PPP networking
    --disable-ppp       Disable PPP networking
    --enable-DISPLAY    Enable I2C Displays
    --disable-DISPLAY   Disable I2C Displays
    --enable-LED_CONTROL
                        Enable MAVLink LED Control
    --disable-LED_CONTROL
                        Disable MAVLink LED Control
    --enable-NOTIFY_NCP5623
                        Enable NCP5623 LED
    --disable-NOTIFY_NCP5623
                        Disable NCP5623 LED
    --enable-NOTIFY_NEOPIXEL
                        Enable NeoPixel
    --disable-NOTIFY_NEOPIXEL
                        Disable NeoPixel
    --enable-NOTIFY_PROFILED
                        Enable ProfiLED
    --disable-NOTIFY_PROFILED
                        Disable ProfiLED
    --enable-NOTIFY_PROFILED_SPI
                        Enable ProfiLED (SPI)
    --disable-NOTIFY_PROFILED_SPI
                        Disable ProfiLED (SPI)
    --enable-PLAY_TUNE  Enable MAVLink Play Tune
    --disable-PLAY_TUNE
                        Disable MAVLink Play Tune
    --enable-TONEALARM  Enable ToneAlarm on PWM
    --disable-TONEALARM
                        Disable ToneAlarm on PWM
    --enable-OSD        Enable OSD
    --disable-OSD       Disable OSD
    --enable-OSD_EXTENDED_LINK_STATS
                        Enable OSD panels with extended link stats data
    --disable-OSD_EXTENDED_LINK_STATS
                        Disable OSD panels with extended link stats data
    --enable-OSD_PARAM  Enable OSD param
    --disable-OSD_PARAM
                        Disable OSD param

@tridge tridge requested a review from peterbarker September 8, 2024 06:49
@tridge tridge force-pushed the pr-build-options-configure branch 4 times, most recently from 3c35af1 to e86d205 Compare September 8, 2024 07:41
@@ -413,7 +421,7 @@ def __init__(self,
Feature('Filesystem', 'FILESYSTEM_SYS', 'AP_FILESYSTEM_SYS_ENABLED', 'Enable @SYS/ filesystem', 0, None),
Feature('Filesystem', 'APJ_TOOL_PARAMETERS', 'FORCE_APJ_DEFAULT_PARAMETERS', 'Enable apj_tool parameter area', 0, None),

Feature('Networking', 'PPP Support', 'AP_NETWORKING_BACKEND_PPP', 'Enable PPP networking', 0, None),
Feature('Networking', 'PPP Support', 'AP_NETWORKING_BACKEND_PPP', 'Enable PPP networking', 0, None, "enable-ppp"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we put these hacks back in waf, please?

Should just be a dictionary mapping from A to B?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it needs to be here to keep backward compatibility with existing documented options like --enable-ppp. We won't need it for new options. It would be an awful lot uglier to have this in wscript

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we're willing to break existing documented configure options we could remove these

Copy link
Contributor

Choose a reason for hiding this comment

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

Or throw a warning in 4.6 that the option is deprecated and remove in 4.7?

Copy link
Contributor

Choose a reason for hiding this comment

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

if we're willing to break existing documented configure options we could remove these

I'm thinking that breaking them would be ok, although it will be a small inconvenience initially for a very small minority who can probably deal with it, because in the long term, part of the power of this feature comes from it's consistency - everything will work the same way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Surely
Feature('Networking', 'PPP Support', 'AP_NETWORKING_BACKEND_PPP', 'Enable PPP networking', 0, None, "enable-ppp"),
should be "PPP" not "PPP Support" - it seems to be the only one with a space in the label field.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to support --enable-ppp, could you not also support --enable-PPP, ditto for ekf2/EKF2, ekf3/EKF3.

I'm thinking that someone might want to build a script that would generate a ./waf configure command line. These special cases would make that difficult, but supporting both (deprecating the lowercase option) might be a good interim solution.

@timtuxworth
Copy link
Contributor

timtuxworth commented Sep 8, 2024

Just a thought about how the options work, could it be

--enable FEATURE or even --enable FEATURE1,FEATURE2,FEATURE3

rather than

--enable-FEATURE

Doesn't that make more sense?

@tridge
Copy link
Contributor Author

tridge commented Sep 8, 2024

--enable FEATURE or even --enable FEATURE1,FEATURE2,FEATURE3

we won't be able to tab complete that, or at least not without a lot of work in the completions logic for each shell. Being able to tab complete options is very nice

@peterbarker peterbarker added the WikiNeeded needs wiki update label Sep 10, 2024
this makes it easy to configure with any option from build_options.py
@tridge tridge force-pushed the pr-build-options-configure branch from e86d205 to 2511395 Compare September 10, 2024 03:55
@tridge
Copy link
Contributor Author

tridge commented Sep 10, 2024

after discussion on the dev call I've removed the backwards compatibility logic

@peterbarker
Copy link
Contributor

after discussion on the dev call I've removed the backwards compatibility logic

We had the forethought to create autotests for the options you've removed :-)

Still, fixing those means we can't easily break this new functionality

@tridge tridge dismissed peterbarker’s stale review September 10, 2024 20:42

removed backwards compatibility

@peterbarker
Copy link
Contributor

@Hwurzburg for the WikiNeeded, we need to remove reference to the removed options --enable-ekf3 and friends (or make it clear they're pre-4.6), and perhaps add a pointer to using the new capabilities for listing configuration options with --help and using them like any other command-line option

@tridge tridge merged commit ef5e3c5 into ArduPilot:master Sep 11, 2024
94 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants