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

Add --save-as param to record #3576

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

theIDinside
Copy link
Contributor

@theIDinside theIDinside commented Aug 15, 2023

--save-as specifies the trace dir name (basename?), not the full path.

Merged output_trace_dir in RecordFlags with name into new type TraceOutputPath.

Changed all usage of output_trace_dir string to use this type instead.

This PR is backwards compatible, in the sense that the old usage of the -o flag remains the same, if --save-as is not provided, i.e., will error out, if dir exists etc. If --save-as is provided together with -o the new behavior will happen instead, where the output dir will be the "root dir", thus, the user can save many traces there.

If only --save-as is provided, record uses normal behavior, of setting the trace root dir to $RR_TRACE_DIR (or it's user provided env var).

Naming of user provided dirs, follows old behavior, i.e. appending -0, -1, -2 etc to the trace dir. I think this is preferable - if some automated thing is recording something with a specific name provided, this makes it so the end user don't have to manage the file system (i.e. checking if that name is "taken" and having to do clean up before recording, etc.) - especially since there's kind of functionality for that, already existing (just using the -o on it's own, though you have to specify a path, not a name)

`--save-as` specifies the trace dir name (basename?), not the full path.

Merged `output_trace_dir` in RecordFlags with `name` into new type `TraceOutputPath`.

Changed all usage of `output_trace_dir` string to use this type instead.

This PR is backwards compatible, in the sense that the old usage
of the `-o` flag remains the same, if `--save-as` is not provided, i.e.,
will error out, if dir exists etc. If `--save-as` is provided together
with `-o` the new behavior will happen instead, where the output dir
will be the "root dir", thus, the user can save many traces there.

If only `--save-as` is provided, record uses normal behavior, of setting
the trace root dir to $RR_TRACE_DIR (or it's user provided env var).

Naming of user provided dirs, follows old behavior, i.e. appending -0,
-1, -2 etc to the trace dir. I think this is preferable - if some
automated thing is recording something with a specific name provided,
this makes it so the end user don't have to manage the file system (i.e.
checking if that name is "taken" and having to do clean up before
recording, etc.)
@theIDinside
Copy link
Contributor Author

theIDinside commented Aug 15, 2023

As a side note @khuey & @rocallahan - I noticed that in the RecordFlags -s is sort of taken - though it's not parsed by the option parsing logic. It's used for always_switch - but if it's not parsed in the option parsing logic, how can it ever be set?

I had originally intended the --save-as to have -s as a short, when I found that it was being used (but not being parsed). Perhaps this needs fixing?

@khuey
Copy link
Collaborator

khuey commented Aug 16, 2023

though it's not parsed by the option parsing logic

Why do you think it's not parsed?

@theIDinside
Copy link
Contributor Author

theIDinside commented Aug 16, 2023

though it's not parsed by the option parsing logic

Why do you think it's not parsed?

Never mind. I had entirely missed that it was specified in the OptionSpec. And I even grepped for it last night. goes to show, don't do things if you're tired. 😛

Either way, I had asked Roc about a feature like this and if it would be acceptable as a PR. I can think of at least one use case where this would be useful, where RR is used to record failing tests (automatically) and being able to specify a name, instead of just the application name (and instead of just a path, that has to be unique) and he OK'd it on Matrix. Let me know what you think of the PR.

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.

None yet

2 participants