ucam-department-of-psychiatry / camcops

Cambridge Cognitive and Psychiatric Test Kit (CamCOPS)
Other
12 stars 8 forks source link

Options to build components separately in the build_qt.py script #357

Closed martinburchell closed 3 months ago

martinburchell commented 3 months ago

The build_qt.py script now has options to turn off building of Qt, OpenSSL, SqlCipher and FFmpeg. I had originally intended to use this when C++ testing but ended up taking another approach. It seems useful functionality to have anyway.

no_build_qt changes meaning from "configure Qt only" to "do not build at all'". A new option configure_qt_only replaces the old meaning of no_build_qt.

@RudolfCardinal I notice that when building the docs if an argument is marked as store_false, the default is marked as True instead of False. Do you know if there is a way around this?

The security failures are addressed in #355

RudolfCardinal commented 3 months ago

For argparse true/false options, the default option can be used -- e.g. from CRATE researcher_report.py:

grp_detail.add_argument(
    "--show_counts",
    dest="show_counts",
    action="store_true",
    default=True,
    help="Include row counts for each table",
)
grp_detail.add_argument(
    "--no_show_counts",
    dest="show_counts",
    action="store_false",
    default=False,
    help="Do not include row counts",
)

Looks good! I've not clicked "approve" in case that makes this comment less obvious, but I do approve!

martinburchell commented 3 months ago

For argparse true/false options, the default option can be used -- e.g. from CRATE researcher_report.py:

Right, so I think the key part of that is if you have a no_xxxx option, which stores a value called xxxx as False when the option is set, you need to set the default to False and have a corresponding xxxx option, which defaults to True.

RudolfCardinal commented 3 months ago

I may have had that slightly wrong. I think the default setting relates to the underlying stored variable. But when two options "speak" to the same variable, it's the default for the first that applies. So if you specify the action="store_true" option before the action="store_false" option, it will work properly. The auto-help is confusing in several ways. Demo below!

#!/usr/bin/env python

import argparse

def main() -> None:
    parser = argparse.ArgumentParser(
        formatter_class=argparse.ArgumentDefaultsHelpFormatter
    )
    help_misleading = " (HELP MAY BE CONFUSING)"

    # Simple:
    parser.add_argument(
        "--foo", action="store_true", help="foo"
        # default (if no argument specified) is: False
    )

    # Also simple:
    parser.add_argument(
        "--bar", action="store_false", help="bar"
        # default (if no argument specified) is: True
    )

    # Odd:
    parser.add_argument(
        "--odd", action="store_false", help="odd", default=False
        # The parameter "odd" now defaults to False, but there is no
        # way to make it true.
    )

    # Less simple:
    parser.add_argument(
        "--baz", dest="baz", action="store_true", help="baz"
    )
    parser.add_argument(
        "--no_baz", dest="baz", action="store_false",
        help="no_baz" + help_misleading
    )
    # Default for "baz" is: False (driven by "store_true").

    parser.add_argument(
        "--no_qux", dest="qux", action="store_false", help="no_qux"
    )
    parser.add_argument(
        "--qux", dest="qux", action="store_true",
        help="qux" + help_misleading
    )
    # Default for "qux" is: True (driven by "store_false").

    # A good way of doing it: making the default explicit.
    parser.add_argument(
        "--explicit", dest="explicit", action="store_true",
        help="explicit", default=False
    )
    parser.add_argument(
        "--no_explicit", dest="explicit", action="store_false",
        help="no_explicit", default=False
    )

    # ... which you can do for either True or False as the default.
    parser.add_argument(
        "--no_explicit2", dest="explicit2", action="store_false",
        help="no_explicit2", default=True
    )
    parser.add_argument(
        "--explicit2", dest="explicit2", action="store_true",
        help="explicit2", default=True
    )

    # But underlying order dependence remains, even when specifying the
    # default, e.g. if you specify two conflicting defaults:
    parser.add_argument(
        "--curious1", dest="curious1", action="store_true",
        help="curious1", default=False
    )
    parser.add_argument(
        "--no_curious1", dest="curious1", action="store_false",
        help="no_curious1", default=True
    )
    # ... defaults to False

    parser.add_argument(
        "--no_curious2", dest="curious2", action="store_false",
        help="no_curious2", default=True
    )
    parser.add_argument(
        "--curious2", dest="curious2", action="store_true",
        help="curious2", default=False
    )
    # ... defaults to True

    args = parser.parse_args()
    print(repr(args))

if __name__ == "__main__":
    main()

# Results:
# Namespace(foo=False, bar=True, odd=False, baz=False, qux=True,
# explicit=False, explicit2=True, curious1=False, curious2=True)

# Therefore:
# - underlying variable default reflects what is specified FIRST unless
#   overridden;
# - help default reflects what is specified for EACH OPTION;
# - that gives rise to some potential for confusion (as does the meaning of
#   the "default" for a "store_false" variable anyway).

Other discussion and references:

There is also a way to make them mutually exclusive:

martinburchell commented 3 months ago

I may have had that slightly wrong. I think the default setting relates to the underlying stored variable. But when two options "speak" to the same variable, it's the default for the first that applies. So if you specify the action="store_true" option before the action="store_false" option, it will work properly. The auto-help is confusing in several ways.

Thanks for that investigation. I think all of the options are now documented correctly, apart from --inherit_os_env / --no_inherit_os_env . This isn't new and the default there is platform-dependent anyway.