Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore unknown flags #5

Merged
merged 1 commit into from
May 18, 2023
Merged

Conversation

mxinden
Copy link
Contributor

@mxinden mxinden commented May 15, 2023

Some of the command line flags provided by the libp2p/test-plans/perf/runner are irrelevant to the quic-go/perf implementation, e.g. the --transport flag. Instead of explicitly ignoring those flags, I suggest ignoring all unknown flags. As far as I can tell the flag package does not provide such option, but the go-flags does.

Mind switching to go-flags here @marten-seemann?

@mxinden
Copy link
Contributor Author

mxinden commented May 17, 2023

Friendly ping. @marten-seemann any objections?

@marten-seemann
Copy link
Member

The problem is that this changes the command line API:

❯ go run cmd/main.go -server-address 127.0.0.1:1234 -run-server=true                                                                  
Usage:
  main

Application Options:
  --run-server        run as server, default: false
  --server-address=   server address, required
  --upload-bytes=     upload bytes
  --download-bytes=   download bytes

Apparently it's now expecting -- instead of -.

@mxinden
Copy link
Contributor Author

mxinden commented May 18, 2023

go-flags allows specifying short flags, though they are only allowed to be one character long.

Is this a show-stopper for you @marten-seemann? Is there someone depending on e.g. -server-address being valid?

As far as I can tell it is unix convention for the single hyphen flag to be one character long. See e.g. man ls.

@marten-seemann
Copy link
Member

I thought this was the required API for the performance tooling, isn't it?

@mxinden
Copy link
Contributor Author

mxinden commented May 18, 2023

Ah, that is the source of the confusion. No, all other implementations use -- instead of -.

@marten-seemann
Copy link
Member

Ok, then this is fine. Thanks for clarifying!

@marten-seemann marten-seemann merged commit 48eac04 into quic-go:master May 18, 2023
@mxinden
Copy link
Contributor Author

mxinden commented May 18, 2023

🙏

mxinden added a commit to mxinden/test-plans that referenced this pull request May 18, 2023
With quic-go/perf#5 and
quic-go/perf#4 one can use the upstream implementation directly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants