Flag refactoring

[jenstopp]

This PR eliminates the use of the pflag package in favor of using the standard flag package. As of now, this is only changed for commands in cmds/core and cmds/exp.

Checking sizes of dependencies [1] indicates that pflag is extraordinary huge compared to flag.

...
2.9 MB mvdan.cc/sh/v3/syntax
2.8 MB github.com/spf13/pflag
2.6 MB reflect
...
560 kB mvdan.cc/sh/v3/expand
559 kB flag
551 kB encoding/asn1
...

Most of the commands use flag anyway, and only a couple use pflag. And most of these only do very basic flag parsing. The major difference between flag and pflag is pflag ability to interpret multiple combined shorthand flags like ls -la for ls -l -a

This is at the same time the only shortcomming, but this appiles only to very few commands in my opinion (ls, cp, ...). One could think of achieving this functionality with a bit extra work on parsing.

File Path	Removed pflag	no global flag parsing	Comment
./cmds/core/basename	[x]	[x]
./cmds/core/cp	[x]	[x]	uses #3009 for multiple combined shorthand flags
./cmds/core/dmesg	[x]	[x]
./cmds/core/fusermount	[x]	[x]
./cmds/core/grep	[x]	[x]	uses #3009 for multiple combined shorthand flags
./cmds/core/ip	[x]	[ ]
./cmds/core/kexec	[x]	[x]	got support for returning flag / uses #3009 for multiple combined shorthand flags / got special handling for kernel-path arg
~./cmds/core/ls~	-	-	done in #3009
./cmds/core/mktemp	[x]	[ ]	uses #3009 for multiple combined shorthand flags
./cmds/core/more	[x]	[ ]
./cmds/core/netstat	[x]	[x]	uses #3009 for multiple combined shorthand flags
./cmds/core/ps	[x]	[ ]	uses #3009 for multiple combined shorthand flags
./cmds/core/rsdp	[x]	[ ]
./cmds/core/shasum	[x]	[ ]
./cmds/core/strings	[x]	[ ]	uses #3009 for multiple combined shorthand flags
./cmds/core/tar	[x]	[ ]	uses #3009 for multiple combined shorthand flags
./cmds/core/tee	[x]	[ ]	uses #3009 for multiple combined shorthand flags
./cmds/core/tr	[x]	[ ]
./cmds/core/ts	[x]	[x]	uses #3009 for multiple combined shorthand flags
./cmds/core/watchdog	[x]	[x]
./cmds/core/watchdogd	[x]	[ ]
./cmds/exp/dmidecode	[x]	[ ]	got support for returning flag
./cmds/exp/esxiboot	[x]	[ ]	got support for returning flag / uses #3009 for multiple combined shorthand flags
./cmds/exp/newsshd	[x]	[ ]	uses #3009 for multiple combined shorthand flags
./cmds/exp/pox	[x]	[x]	uses #3009 for multiple combined shorthand flags

[1] Size analysis: go version go1.22.1 linux/amd64 goweight, commit 624b44a (https://github.com/jondot/goweight) makebb (gobusybox), commit d8fbaca

1. Run makebb -k ./cmds/core/* ./cmds/exp/* with the cmds adapted to your needs
2. Note from output
3. Run GOPATH=<temp dir> GO111MODULE=off goweight <temp dir>/src/bb.u-root.com/bb/main.goz // Setting of GOPATH and GO111MOD is due to the way gobusibox works and not related to how goweight works. goweight works when go build works.

[jenstopp]

Unfortunately, in the end (after stripped binary, DCE, ..) the impact on the size of the busybox is not that huge.

[rminnich]

The ONLY issue with flag is this:

./cmds/core/grep	[x]	may need a solution for multiple combined shorthand flags

we started with pflag in the first place because of this problem. The golang flag package is a plan 9-inspired design and it's never been very good.

I'd like to get rid of all non-standard flag packages (see #3003) but I don't see a way to get rid of all uses :-(

Go needs a better flag package. It's never been good.

[rminnich]

Please see #3009, which will give us more unix-like flags. If that is ok, we can remove spf13 and for programs like grep, ps, etc. add util.UnixParse

[rminnich]

please take another look at #3009, it now includes removing spf13 from the ls command.

[rminnich]

FYI, not sure what tool you are using, but gofmt can do this pretty nicely, e.g. gofmt -w -r 'flag.BoolP(a, b, c, d) -> flag.Bool(b, c, d)' will rewrite flag.BoolP and delete the first arg.

gofmt -w -r 'flag.BoolP(a, b, c, d) -> flag.Bool(b, c, d)' tr.go
rminnich@pop-os:~/go/src/github.com/u-root/u-root/cmds/core/tr$ git diff
diff --git a/cmds/core/tr/tr.go b/cmds/core/tr/tr.go
index fa73d4c94..d056d7333 100644
--- a/cmds/core/tr/tr.go
+++ b/cmds/core/tr/tr.go
@@ -45,7 +45,7 @@ import (
        flag "github.com/spf13/pflag"
 )

-var del = flag.BoolP("delete", "d", false, "delete characters in SET1, do not translate")
+var del = flag.Bool("d", false, "delete characters in SET1, do not translate")

sort of like spatch but easier.

[rminnich]

you can take ls out of this PR, it's done already

[rminnich]

my suggestion: I don't think we want spf13. I think this change should be done one function at a time, however, to make bisection easier if we break something.

So ... if you are ok with it ... I will proceed, with the gofmt thing shown above, and create a series of commits rolling back the spf13 bits, until we can just remove use of the package. WDYT?

[jenstopp]

my suggestion: I don't think we want spf13. I think this change should be done one function at a time, however, to make bisection easier if we break something.

I did separate commits per command here for the same reason. So I can also review them and introduce UnixParse where necessary?

[rminnich]

ah, your approach is fine too. Can you remove the ls commit?

[hugelgupf]

How about

unixflag.Parse:

func Parse() { flag.CommandLine.Parse(OSArgsToUnixArgs()) }

Replace all flag.Parse with unixflag.Parse.

On Mon, Jun 17, 2024 at 12:17 ron minnich @.***> wrote:

ah, your approach is fine too. Can you remove the ls commit?

— Reply to this email directly, view it on GitHub https://github.com/u-root/u-root/pull/3004#issuecomment-2174242667, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPG3EWVWZO4LOT7Z6GNB23ZH4Y47AVCNFSM6AAAAABJIBKBVWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZUGI2DENRWG4 . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[jenstopp]

ah, your approach is fine too. Can you remove the ls commit?ah, your approach is fine too. Can you remove the ls commit?

Sure! Also need to do the other small fix ups as mentioned in the overview. And then apply Chris' idea.

Will continue on that tomorrow.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 74.23208% with 151 lines in your changes missing coverage. Please review.

Project coverage is 58.85%. Comparing base (d181bf1) to head (2563aee).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3004      +/-   ##
==========================================
+ Coverage   58.35%   58.85%   +0.50%     
==========================================
  Files         534      534              
  Lines       33208    33432     +224     
==========================================
+ Hits        19377    19676     +299     
+ Misses      13831    13756      -75     

Flag	Coverage Δ
.-amd64	90.69% <ø> (ø)
cmds/...-amd64	48.15% <70.95%> (+1.05%)	:arrow_up:
integration/generic-tests/...-amd64	20.19% <74.68%> (+0.38%)	:arrow_up:
integration/generic-tests/...-arm	11.69% <ø> (ø)
integration/generic-tests/...-arm64	24.46% <69.62%> (+0.50%)	:arrow_up:
integration/gotests/...-amd64	62.09% <71.04%> (+0.28%)	:arrow_up:
integration/gotests/...-arm	62.92% <71.04%> (+0.06%)	:arrow_up:
integration/gotests/...-arm64	62.99% <71.04%> (-0.08%)	:arrow_down:
pkg/...-amd64	58.99% <100.00%> (+0.03%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
everything	64.84% <76.28%> (+0.49%)	:arrow_up:
cmds/exp	31.76% <67.62%> (+0.62%)	:arrow_up:

[jenstopp]

Removed ls from the PR, since already done. Finished work on the three commands, that need support for returning flags (kexec, dmidecode, esxiboot). I therefore extended pkg/uroot/unixflag. Alternative approached are: a) use/adapt https://github.com/u-root/gobusybox/blob/main/src/pkg/uflag/uflag.go - reduce code duplication somehow (however, it's from a separate repo) b) define an mplementation of flag.Var in the respective cmds itself when needed - Will eventually prevent from pkg/uroot/unixflag from being bloated when similar cases approach (IntArray, MayVarArray, ..) and instead solve this issues then on demand

[jenstopp]

unixflag.Parse:

func Parse() { flag.CommandLine.Parse(OSArgsToUnixArgs()) }

Replace all flag.Parse with unixflag.Parse.

This would avoid f := flag.NewFlagSet(args[0], flag.ExitOnError) like done in ls now, which is kinda cleaner as there is no need for a custom flagset, actually.

However, not sure if be no means replace all flag.Parse with unixflag.Parse in all cmds or just where need with a comment.

[rminnich]

I don't think you need unixflag.Parse any more, that was an earlier discussion, sorry about confusion I caused.

I think ls.go shows a path forward without any globals.

[10000TB]

@rminnich @ChriMarMe @binjip978 @hugelgupf any other concern ? -- need an at least one approval from you to proceed.

[jenstopp]

@rminnich @hugelgupf what's your opinion on this:

Finished work on the three commands, that need support for returning flags (kexec, dmidecode, esxiboot). I therefore extended pkg/uroot/unixflag. Alternative approached are: a) use/adapt https://github.com/u-root/gobusybox/blob/main/src/pkg/uflag/uflag.go

• reduce code duplication somehow (however, it's from a separate repo) b) define an mplementation of flag.Var in the respective cmds itself when needed
• Will eventually prevent from pkg/uroot/unixflag from being bloated when similar cases approach (IntArray, MayVarArray, ..) and instead solve this issues then on demand

[jenstopp]

Also, @rminnich @hugelgupf could you take a look at the cmdline parsing in kexec. It passes all tests (incl. integration) but though it works well, it's still a bit hacky. Left some comments there in the code.

If you have better ideas, I'd refactor that in a separate PR.

[jenstopp]

This PR reduced the sice of a compressed initramfs incl. cmds/core/* and cmds/exp/* as follows: GOARCH='amd64' GOOS='linux'

git checkout d181bf1
go build -a
./u-root ./cmds/core/* ./cmds/exp/*
gzip initramfs.cpio
du initramfs.cpio.gz

-> 6380

git checkout 443f1dd
go build -a
rm initramfs.cpio.gz
./u-root -o initramfs.cpio ./cmds/core/* ./cmds/exp/*
gzip initramfs.cpio
du initramfs.cpio.gz

-> 6372

(6380 - 6372) / 6380 * 100% = 0,13%

[10000TB]

NVM: mid-read.

~@jensdrenhaus

(6380 - 6372) = 8 ?

so 8 / 6380 * 100% = 0.00125 * 100 % = 0.125 %~