Upgrade bpaf to 0.7 to remove some turbofishes

[pacak]

Plus various code improvements, feel free to drop some or all of them.

[pacak]

Do you want to update the code to use derive macro? This means compiling syn, but it's already being compiled...

[Shnatsel]

Thanks! I'll take a look shortly.

Re: derive macro: I'm less concerned about compiling syn as I am about the macro being re-run on every compilation, increasing the incremental compilation times. I'm not very familiar with how derive macros work, so if that's not an issue, then using a derive macro to reduce the amount of code would be great.

[pacak]

https://github.com/rosetta-rs/argparse-rosetta-rs according to this other than initial cost to compile syn and related stuff - there's no difference to incremental compile time.

[Shnatsel]

In that case using the derive API would be great! We already use syn for serde, so we just get to maintain less boilerplate for free.

[pacak]

In that case using the derive API would be great!

Okay. Gimme a day or two.

[Shnatsel]

Thanks a lot!

[pacak]

Pushed, wording might change in some places but cli tests are passing as is so hopefully nothing breaks.

Also added a way for users to make cli parser output more colorful a-la clap3/clap4 if they want to. No extra dependencies by default.

[Shnatsel]

It's a little strange to resolve color as a compilation feature. This is usually runtime configuration - and is disabled by default when writing to something other than a terminal, so that the plain text file doesn't end up littered with terminal escape sequences. See e.g. the behavior of grep --color= where it defaults to auto, but can be explicitly enabled or disabled.

Is there a way to do runtime resolution with bpaf? If not, I'd rather stick to plain/colorless output so that the output dumped to a text file would still be readable.

[pacak]

Runtime detection is present as well and will be used if you enable either colorscheme so redirect to file or running in CI would work as expected. Having it as a compile time helps people to decide if they want any colors at all - I would prefer no colors so won't be enabling this feature, but some people like it.

[Shnatsel]

My apologies for the delay, I got caught up in RL stuff. I'll try to review and merge this next week.

[pacak]

My apologies for the delay, I got caught up in RL stuff. I'll try to review and merge this next week.

No worries, take your time.

[pacak]

Any thoughts? Also do you want a pull request for cargo-auditable? pico-args is simple but correctness is not quite there - see https://github.com/RazrFalcon/pico-args/issues/15 for example.

[Shnatsel]

Thank you for the reminder, I'll try to take a look in the next few days now that I've shipped cargo audit 0.17.3.

cargo auditable doesn't need any help generation, and it only captures a subset of all arguments passed. Basically I'm intercepting only some of the arguments passed to rustc, for which pico-args is a great fit.

I am aware of the correctness issue you mentioned, but it only matters for invalid argument sequences, and rustc itself already does an argument validation pass so we don't have to.

[Shnatsel]

Looks good to me! I'm going to merge this and cut a release. Thanks a lot!

I'm going to make the stylistic choice of bright colors, and drop the feature. I think that's the option that's easiest to skim, and the colors chosen are readable on both light and dark backgrounds. Handling of piping also looks correct to me.

[pacak]

I'm going to make the stylistic choice of bright colors, and drop the feature.

There's no downside to leaving the feature in even if you go with the bright colors by default, for example I would prefer not having color at all compared to the bright option. Visibility depends on only on background being dark or bright, but also on a palette.

[pacak]

https://github.com/clap-rs/clap/discussions/2906#discussioncomment-1646705

[pacak]

Also if you really want to drop the color selection - you need to update the docs.

[Shnatsel]

Actually, I'll have to disable color for now because it pulls in atty, which has known UB and appears to be unmaintained: https://github.com/softprops/atty/issues/50

[pacak]

Bummer. I'm not using it directly but through owo-colors. Do you know of any working/maintained alternative?

On Fri, Nov 4, 2022, 16:17 Shnatsel @.***> wrote:

Actually, I'll have to disable color for now because it pulls in atty, which has known UB and appears to be unmaintained: softprops/atty#50 https://github.com/softprops/atty/issues/50

— Reply to this email directly, view it on GitHub https://github.com/rust-secure-code/cargo-supply-chain/pull/92#issuecomment-1304159608, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAQFI4FVEXKX3BPLYHOTBLWGVVN3ANCNFSM6AAAAAARCOA2OM . You are receiving this because you authored the thread.Message ID: @.***>

[Shnatsel]

No, but I haven't really looked.

You could always just fork it and we can encourage people to switch via the RustSec advisory for example: https://github.com/rustsec/advisory-db/issues/1457

[Shnatsel]

I've shipped v0.3.2 - without color, but it does update to bpaf 0.7.x on crates.io.

I'd be happy to add color later because I think it meaningfully improves the readability of the help text.

[pacak]

I'll see what I can do about atty - I have some other changes planned first. Maybe a week or two.

[pacak]

I did this for now FWIW https://github.com/zkat/supports-color/issues/9

[pacak]

supports-color pull request got merged, hopefully a release soon.