Ticket #43972

generate_packets.py: Make output paths for each file optional

Open Date: 2022-02-22 23:04 Last Update: 2022-02-26 22:16

Reporter:
Owner:
Type:
Status:
Closed
Component:
MileStone:
Priority:
5 - Medium
Severity:
5 - Medium
Resolution:
Fixed
File:
1

Details

Part of #43927. Followup to #43971. Sometimes (like current CI meson build) it might be desirable to only write some, not all, of the output files. This should be officially supported by making the respective path arguments optional, rather than by passing empty paths.

Ticket History (3/5 Histories)

2022-02-22 23:04 Updated by: alienvalkyrie
  • New Ticket "generate_packets.py: Make output paths for each file optional" created
2022-02-24 07:31 Updated by: alienvalkyrie
Comment

Doing this properly is nontrivial. Just making the positional arguments optional would mean it's only possible to omit the later arguments, not the earlier ones. We need to add --name options for each path.

There are two ways we could go about this:

  • The ugly, complicated, but backwards-compatible way, which is to allow both positional and named arguments; doing this in a way that reliably works, produces understandable help text (without having to manually write it) and understandable error messages will probably be somewhat convoluted.
  • The quick, clean, but not backwards-compatible way, which is to remove the positional arguments and completely replace them with named arguments; this would also require changing the relevant calls in the build system (meson.build and common/Makefile.am, as far as git grep can tell), and it would break anyone else's custom builds. (Though, given the helpful usage instructions printed by argparse, most people should be able to easily fix their builds.)

(I assume one can tell from my choice of words which one I'd prefer, but to spell it out – unless we desperately need backwards compatibility here, I'd go with the second option.)

There's also the question of whether we'd still allow passing empty strings as an alternative to omitting arguments; IMO that decision should mirror the first one (i.e. keep allowing it only if we choose to maintain backwards compatibility).

2022-02-25 08:13 Updated by: alienvalkyrie
  • Owner Update from (None) to alienvalkyrie
  • Resolution Update from None to Accepted
Comment

Attached patch takes the second option, i.e. breaks backwards compatibility for potential custom builds. If there is any reason why we need to maintain backwards compatibility here, now would be a good time to mention it.

2022-02-26 22:16 Updated by: alienvalkyrie
  • Status Update from Open to Closed
  • Resolution Update from Accepted to Fixed

Edit

You are not logged in. I you are not logged in, your comment will be treated as an anonymous post. » Login