Parser set defaults improvements

[AndrewD]

Improvements to LiteXArgumentParser when using set_defaults() in a target to ensure args_fill() for the new cpu_type is called and to report using set_default(arg=default) for an argument that doesn't exist.

[AndrewD]

This is required for this to work:

parser.set_defaults(cpu_type="vexriscv_smp")
parser.set_defaults(cpu_variant="linux")
parser.set_defaults(with_fpu=True)

[trabucayre]

Having a test and/or a message looks good, but raising an exception may, potentially being a problem for users:

• if in a target I set cpu_type=vexriscv_smp and dtlb_size=xxx
• if a user decide to use --cpu-type=naxriscv

An exception will occur because dtlb_size is something specific to the vexriscv_smp.

Maybe it's better here to displays a message (similar to compat_notice) with unused/unknown arguments list with and a delay to warn user about situation. Otherwise user has to modifiy target to remove/comment default and it's not really user friendly.

[trabucayre]

To complete my previous message (because I have, maybe,seen why you have added the raise KeyError).
If my understanding is true it's to mimic, for set_defaults, argparse's behavior, no?
In fact I see three cases:

• a user/dev adds some set_defaults in the target (for CPU configuration and/or to select/configure toolchain): here we assume he know what it do, and with a warning/delay it will able to fix a potential typo.
• a user select some CLI's arguments: if there is a typo or unknown argument: it make sense to stop because it's an unwanted situation: it's not possible to build gateware with or without parameters selected by user, due to a typo.
• last possibily is when user select/change, with CLI, some parameters affecting set_defaults: we can consider user know why this change, and if a defaults value/argument is no more relevant, it make sense to warn but its not an error (in my mind).

[AndrewD]

Thanks for the detailed review.

From what I've seen, there are many cases where the litex behaviour is to raise an exception of something goes wrong so this pattern is not unusual.

However I don't remember my exact thought process on this as I added it a long time ago. I don't mind how the error is reported, I simply wanted it to be clear that the arguments were incorrect instead of being silently ignored as a typo in set_default() is otherwise far harder to find and I lost some time on several occasions due to this, hence I added something to detect the issue and stop.

Ultimately this exception is only thrown if there is a bug in the target file.

For example I guard set_defauts() for cpu specific arguments based on cpu_type so this exception can be resolved by writing a sane target 😀

[AndrewD]

For example this is how I use set_defaults() for cpu_type to guarantee the defaults are valid while also generating a target that will, by default, build with our required features:

if not any("--cpu-type" in arg for arg in sys.argv):
    parser.set_defaults(cpu_type="vexriscv_smp")
    parser.set_defaults(cpu_variant="linux")
    parser.set_defaults(with_coherent_dma=True)
    parser.set_defaults(with_rvc=True)
    parser.set_defaults(with_fpu=True)
    parser.set_defaults(with_privileged_debug=True)
    parser.set_defaults(with_wishbone_memory=True)
    parser.set_defaults(expose_time=True)
    parser.set_defaults(hardware_breakpoints=4)

There are a range of other arg parsing refinements I also made a while ago in an PR here: #1759. This small piece was an attempt to break the original larger change set into smaller chunks to be easier to review and merge.

[trabucayre]

For example this is how I use set_defaults() for cpu_type to guarantee the defaults are valid while also generating a target that will, by default, build with our required features:

if not any("--cpu-type" in arg for arg in sys.argv):
    parser.set_defaults(cpu_type="vexriscv_smp")
    parser.set_defaults(cpu_variant="linux")
    parser.set_defaults(with_coherent_dma=True)
    parser.set_defaults(with_rvc=True)
    parser.set_defaults(with_fpu=True)
    parser.set_defaults(with_privileged_debug=True)
    parser.set_defaults(with_wishbone_memory=True)
    parser.set_defaults(expose_time=True)
    parser.set_defaults(hardware_breakpoints=4)

Yes it's always possible to do something like that, but it adds a bit more complex and break the idea behind LiteXArgumentParser: reducing complexity at targets level. It also possible to think to a set_defaults variant with a condition to only inject this when cond is True (but it's beyond this PR scope).

There are a range of other arg parsing refinements I also made a while ago in an PR here: #1759. This small piece was an attempt to break the original larger change set into smaller chunks to be easier to review and merge.

Yes it always better to have a small patches serie (ie multiple PR) with small modifications/fix/new features and a commit message enough to see why this PR (something similar to a primary school's math demonstration) : it's simplify the task for maintainers/reviewers and allows to be merged more quickly.

For this PR, first part looks good because it fix a specific issue where cpu-type is not taken into account when it must: with some potential issue for CPU configuration. For the second part I'm a bit mitigated because it make sense to help developer to fix some typos but in the same time it will introduce a potential build failure when user select parameters different than default:

• developer know what it do and if a warning is displayed may fix is code
• user is not always aware about targets content and may not be able to see/know why build fails due to an option not provided in the command line.

[AndrewD]

I can drop the second patch from this pr if that helps?
But hardly any targets use set_default at the moment so it's safe from that regard 😂

[enjoy-digital]

@AndrewD, @trabucayre: I think we can merge as is since already fixing issues with CPU defaults and since mostly experienced developpers will be using the set_defaults for fixed configuration, raising an error when the argument is not supported seems fine.

I'm merging and will do a more global review when I'll have a bit more time.

[AndrewD]

Thanks @trabucayre and @enjoy-digital !