Apply bundle.yml options in command-line order

[cresswellp]

Description

Command line options defined in bundle.yml are always applied in the order they're processed by argparse, i.e. in the order they're defined in the bundle. This means that e.g. if two options with-foo and without-foo set BUILD_foo=ON and BUILD_foo=OFF respectively, then both --with-foo --without-foo and --without-foo --with-foo will produce the same result, which depends only on which option is defined first in bundle.yml. This is an inherent restriction of argparse.

Processing the bundle.yml options (and only the bundle.yml options) in the order they're passed to ecbundle removes this restriction. This doesn't affect all command line options, e.g. arguments passed by --cmake will still be processed after any bundle-defined options, but this seems to provide sufficient flexibility for users to be able to obtain the desired result.

Contributor Declaration

By opening this pull request, I affirm the following:

• All authors agree to the Contributor License Agreement.
• The code follows the project's coding standards.
• I have performed self-review and added comments where needed.
• I have added or updated tests to verify that my changes are effective and functional.
• I have run all existing tests and confirmed they pass.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 64.70588% with 6 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (develop@412dfa1). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
ecbundle/build.py	45.45%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             develop      #31   +/-   ##
==========================================
  Coverage           ?   40.71%           
==========================================
  Files              ?       48           
  Lines              ?    10133           
  Branches           ?        0           
==========================================
  Hits               ?     4126           
  Misses             ?     6007           
  Partials           ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[wdeconinck]

Thank you for the suggestion.
Why would this be an acceptable configuration? It is indeed ambiguous what the user wants.
Possibly it's a better to abort and not allow to specify two contradicting options in the first place, forcing to fix this at the callsite. That is however hard to do with programmable options like this.

Also this class BundleBuilder should not have any idea of the command line options as in sys.argv.
What if sys.argv does not contain an option provided in the bundle?

[cresswellp]

Why would this be an acceptable configuration?

In a system as complicated as the IFS we have lots of places where options are added as defaults, but the user can negate them; it's not the user that would be explicitly adding both options, but choosing to override a default. At the moment we're managing by carefully planning what defaults are in place and thus which options will be the 'overrides' and thus should come second, but it all feels rather precarious. Something like this would make things a lot more robust.

Also this class BundleBuilder should not have any idea of the command line options as in sys.argv. What if sys.argv does not contain an option provided in the bundle?

I understand exposing sys.argv isn't ideal, but I couldn't see a better solution (I'm open to suggestions!). Unless I've made a mistake - I did test the changes against the IFS - if there are no bundle options in the argument list then this block will do nothing, the same as before.

[cresswellp]

As an attempt to find a solution that works for everyone - if I could achieve the same result cleanly by extending the ArgumentParser class, without exposing argv, would that be acceptable? This functionality really would be useful when we have multiple ways of wanting to run the IFS, each requiring different default setups.

[wdeconinck]

Hi @cresswellp , if you mean that you can modify/extend the Argparse class so it would be able to take care of duplicate entries, and overwriting the previous ones in order that would be great!