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.

Sign up for GitHub

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

Jump to bottom

Add support for response files #2320

Merged
merged 3 commits into from
Jun 30, 2023
Merged

Add support for response files #2320

merged 3 commits into from
Jun 30, 2023

Conversation

kant2002
Copy link
Contributor

@kant2002 kant2002 commented Jun 5, 2023

I make workaround here for now, but essentially issues in CommadLine library. Also place workardoud for stupid bug with quoted argumetns with space in them.

I notice issue with parsing and request for RSP on Discord where @kg and @radical discuss how to feed proper parameters in benchmarks_ci.py

@kant2002
Add support for response files
d00d191 
I make workaround here for now, but essentially issues in CommadLine library.
Also place workardoud for stupid bug with quoted argumetns with space in them.

I notice issue with parsing and request for RSP on Discord where @kg and @radical discuss how to feed proper parameters in `benchmarks_ci.py`
@kg
Copy link
Member

kg commented Jun 5, 2023

It looks like this addresses the existing problem of --wasmArgs \"--expose-wasm --module\" not parsing correctly while --wasmArgs \" --expose-wasm --module \" does? Neat.

@kant2002
Copy link
Contributor Author

kant2002 commented Jun 5, 2023

Yes, I did create workaround by injecting whitespace in BDN. For sure this should be fixed in CommandLine library, but I do not sure reaction of time of their maintainers. also coincidentally --wasmArgs only parameter which seems to be need whitespace, so nobody notice issue. This issue still present in CommandLine 2.9

I thought that quality of life for BDN is more important then proper fix. Will try to submit real fix a bit later.

kant2002 added a commit to kant2002/commandline that referenced this pull request Jun 6, 2023
@kant2002
Fix parsing values which starts with dash, but has whitespace inside.
9be1ee8 
this is required when your option can accept command line arguments to other applications.

See dotnet/BenchmarkDotNet#2320 (comment) for example of parsing issues
@kant2002 kant2002 mentioned this pull request Jun 6, 2023
Open
@AndreyAkinshin
Copy link
Member

@kant2002 thanks for the PR. As I can see, this PR suggests reserving the @ argument prefix for the response files. Do we really need a new type of notation for such a feature? Could we use the existing notation (e.g., introduce the --reponse-file argument)?

@adamsitnik what do you think?

@kant2002
Copy link
Contributor Author

kant2002 commented Jun 7, 2023

@ is well known convention from compiler world. And also supported by System.CommandLine (not that this is matter much to me) which means eventually it would propagate across ecosystem. Also if use --response-file that would make it “magic” argument which somehow should be documented in Options but applied in different place (same as in this PR).

@kant2002
Copy link
Contributor Author

@adamsitnik I would like to hear your opinion on the matter. Are you have time for take a look at PR ?

AndreyAkinshin
AndreyAkinshin requested changes Jun 30, 2023
View reviewed changes
src/BenchmarkDotNet/ConsoleArguments/ConfigParser.cs Outdated
if (arg.StartsWith("@"))
{
var fileName = arg.Substring(1);
if (File.Exists(fileName))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the given file doesn't exist? I believe we should warn the user about that rather than just silently ignore such an argument.

src/BenchmarkDotNet/ConsoleArguments/ConfigParser.cs Outdated
var fileName = arg.Substring(1);
if (File.Exists(fileName))
{
var lines = File.ReadAllLines(fileName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add try..catch with proper exception handling? If there are some issues with the given file (e.g., we don't have permission to read the file content), it would be nice to print an error message rather than just throwing an exception.

@AndreyAkinshin
Copy link
Member

@kant2002 I think I'm OK with introducing such a @-syntax. You are right, such an approach is a common convention for many tools. Could you please address the above comments from the review and add some docs covering this feature?

@AndreyAkinshin AndreyAkinshin merged commit 977c05e into dotnet:master Jun 30, 2023
@AndreyAkinshin
Copy link
Member

@kant2002 thanks!

@kant2002 kant2002 deleted the kant/rsp-support branch July 1, 2023 03:43
@AndreyAkinshin AndreyAkinshin added this to the v0.13.6 milestone Jul 4, 2023
@timcassell timcassell mentioned this pull request Jul 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndreyAkinshin AndreyAkinshin AndreyAkinshin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.13.6
Development

Successfully merging this pull request may close these issues.
3 participants
@kant2002 @kg @AndreyAkinshin