Add --save-as param to record
#3787
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rocallahan
requested changes
Jul 21, 2024
| @@ -663,6 +663,12 @@ void replace_in_buffer(MemoryRange src, const uint8_t* src_data, | |||
|
|
|||
| // Strip any directory part from the filename `s` | |||
| void base_name(std::string& s); | |||
| struct TraceOutputPath { | |||
| std::string output_trace_dir; | |||
There was a problem hiding this comment.
Please carefully document these fields.
There was a problem hiding this comment.
See new comments added to the type - are they enough?
|
@theIDinside can you please debug the new elements in util.h as requested and rebase, so that we may see that be integrated? |
`--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.) Added test cases
Rebased it and added documentation to the fields |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a resend of PR ##3576 because I messed that one up @GitMensch || @rocallahan
This PR is rebased onto master and also added test cases for the feature.
--save-asspecifies the trace dir name (basename?), not the full path.Merged
output_trace_dirin RecordFlags withnameinto new typeTraceOutputPath.Changed all usage of
output_trace_dirstring to use this type instead.This PR is backwards compatible, in the sense that the old usage of the
-oflag remains the same, if--save-asis not provided, i.e., will error out, if dir exists etc. If--save-asis provided together with-othe 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-asis 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.)
Added test cases