cli: Add new history command #1667

AbrarQuazi · 2024-11-05T17:33:14Z

Added new wake --history command

AbrarQuazi · 2024-11-05T17:38:03Z

How the history command looks like. I used the default date time formatting from the Time library that we have for consistency, but if needed, I can change the formatting to something else like [Mon Nov 27 12:17:37 PST 2023] (but to do this new logic needs to be added to get the abbreviated days and month for processing as sqlite library doesnt do this out of the box)

AbrarQuazi · 2024-11-05T17:40:45Z

src/runtime/database.cpp

@@ -396,6 +398,7 @@ std::string Database::open(bool wait, bool memory, bool tty) {
  const char *sql_tag_job = "insert into tags(job_id, uri, content) values(?, ?, ?)";
  const char *sql_get_tags = "select job_id, uri, content from tags where job_id=?";
  const char *sql_get_all_tags = "select job_id, uri, content from tags";
+  const char *sql_get_runs_table = "select run_id, time, cmdline from runs order by time ASC";


I could have gotten sqlite to process the time to appropriate datetime format like SELECT strftime('%Y-%m-%d %H:%M:%S', time / 1000000000, 'unixepoch'), but decided to just default to the Time Library

wake/src/runtime/database.h

Line 55 in b468938

struct Time {

AbrarQuazi · 2024-11-05T17:49:18Z

tools/wake/main.cpp

+  const auto runs = db.get_runs();
+  for (const auto run : runs)
+  {
+    std::cout << run.time.as_string() << " " << run.cmdline << std::endl; 


Decided to just output the string via std::cout, but I could have used the Describe library instead like we do with query_jobs :

wake/tools/wake/describe.cpp

Line 371 in b468938

void describe(const std::vector<JobReflection> &jobs, DescribePolicy policy, const Database &db) {

. But to do this, it would require additional refactoring the whole file to accept not only a JobReflection but now a RunReflection as well (could make a TableReflection parent class that describe takes in for polymorphism). Would that type of change be of benefit and needed now?

colinschmidt · 2024-11-05T20:13:18Z

I used the default date time formatting from the Time library that we have for consistency

Where else do we print timestamps? I'm not sure we need the fancier formatting you mentioned but was thinking we could just drop the subsecond precision. Most wake invocations take longer than a second so I don't think we would lose any meaningful precision.

AbrarQuazi · 2024-11-05T21:39:16Z

Where else do we print timestamps

The only other time we use Time 's string function is for wake --script. Don't think subsecond is required for that right?

ag-eitilt

+0.75 for dropping the subseconds. Even for jobs which do take less than a second, that's still not affecting that statistic directly (which is helpful to have), it's just saying when the job was launched, and I don't see much reason for that to matter. The big caveat I have is that subsecond resolution should still be visible when the --timeline is zoomed in far enough (I think that's how it can be interacted with, right? Shows how little I use that feature.) -- if dropping the resolution at the struct level means the timeline gets quantized to seconds, or to a lesser degree if the JSON is affected in the same way, then we'd want to be a lot more hesitant about it.

src/runtime/database.cpp

ag-eitilt · 2024-11-06T17:29:58Z

src/runtime/database.cpp

@@ -1584,6 +1589,25 @@ std::vector<JobTag> Database::get_tags() {
  return out;
 }

+static std::vector<RunReflection> get_all_runs(const Database *db, sqlite3_stmt *query) {


... but here you're using that pattern already ...

(Side note, this seems to be the only implementation here which uses a separate static function rather than having it all in the Database:: method.)

Sounds good, yeah removed the static function ( I was just following the get_file_dependencies pattern:

wake/src/runtime/database.cpp

Line 1626 in 4d62b1d

return get_all_file_dependencies(this, imp->get_file_dependency);

, but realized that is unnecessary for get_runs)

src/runtime/database.h

tools/wake/main.cpp

src/runtime/database.h

colinschmidt · 2024-11-06T22:17:38Z

src/runtime/database.h

+  int id;
+  Time time;
+  std::string cmdline;
+  RunReflection() = default;


Do we need this constructor?

We dont necessarily need it, but I used the constructor so its clearly understood what exactly I am querying:
https://github.com/sifive/wake/pull/1667/files#diff-ce97135c77b7c227bcd205cb4d3449133611307c21539f37b8c422bde9780252R1591-R1593

Ok thats fair. It we don't have any style convention here thats fine.

tools/wake/main.cpp

colinschmidt · 2024-11-06T22:20:31Z

tools/wake/main.cpp

@@ -639,7 +652,7 @@ int main(int argc, char **argv) {
  }

  if (is_db_inspection) {
-    inspect_database(clo, db, wake_cwd);


This argument was just dead code already?

Yeah it was already dead code

AbrarQuazi · 2024-11-06T22:26:28Z

e --timeline is zoomed in far enough (I think that's how it can be interacted with, right

I dont see --timeline really interacting with the --script command or calling Time's as_string() function

colinschmidt

Some small spelling nits but otherwise LGTM.

colinschmidt · 2024-11-12T22:23:12Z

src/runtime/database.h

+  int id;
+  Time time;
+  std::string cmdline;
+  RunReflection() = default;


Ok thats fair. It we don't have any style convention here thats fine.

colinschmidt · 2024-11-12T22:26:25Z

tools/wake/main.cpp

@@ -268,6 +279,7 @@ void print_help(const char *argv0) {
    << "    --last     -l      See --last-used"                                            << std::endl
    << "    --last-used        Capture all jobs used by last build. Regardless of cache"   << std::endl
    << "    --last-executed    Capture all jobs executed by the last build. Skips cache"   << std::endl
+    << "    --history          Report the cmndline history of all wake commands recorded"  << std::endl


Suggested change

<< " --history Report the cmndline history of all wake commands recorded" << std::endl

<< " --history Report the command-line history of all wake commands" << std::endl

To match the spelling for --in
We don't need to specify "recorded" in the same way we don't specify that for things like "--list-outputs"

cli: Add new history command

b468938

AbrarQuazi requested a review from colinschmidt November 5, 2024 17:33

AbrarQuazi commented Nov 5, 2024

View reviewed changes

Run clang

4d62b1d

ag-eitilt reviewed Nov 6, 2024

View reviewed changes

address pr comments

4dd91e2

colinschmidt reviewed Nov 6, 2024

View reviewed changes

update tagdag function

a94ede3

colinschmidt approved these changes Nov 12, 2024

View reviewed changes

AbrarQuazi merged commit b169e45 into master Nov 12, 2024
11 checks passed

AbrarQuazi deleted the add-wake-history branch November 12, 2024 23:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: Add new history command #1667

cli: Add new history command #1667

AbrarQuazi commented Nov 5, 2024

AbrarQuazi commented Nov 5, 2024

AbrarQuazi Nov 5, 2024 •

edited

Loading

AbrarQuazi Nov 5, 2024

colinschmidt commented Nov 5, 2024

AbrarQuazi commented Nov 5, 2024 •

edited

Loading

ag-eitilt left a comment

ag-eitilt Nov 6, 2024

AbrarQuazi Nov 6, 2024

colinschmidt Nov 6, 2024

AbrarQuazi Nov 6, 2024

colinschmidt Nov 12, 2024

colinschmidt Nov 6, 2024

AbrarQuazi Nov 6, 2024

AbrarQuazi commented Nov 6, 2024

colinschmidt left a comment

colinschmidt Nov 12, 2024

colinschmidt Nov 12, 2024

	<< " --history Report the cmndline history of all wake commands recorded" << std::endl
	<< " --history Report the command-line history of all wake commands" << std::endl

cli: Add new history command #1667

cli: Add new history command #1667

Conversation

AbrarQuazi commented Nov 5, 2024

AbrarQuazi commented Nov 5, 2024

AbrarQuazi Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinschmidt commented Nov 5, 2024

AbrarQuazi commented Nov 5, 2024 • edited Loading

ag-eitilt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbrarQuazi commented Nov 6, 2024

colinschmidt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbrarQuazi Nov 5, 2024 •

edited

Loading

AbrarQuazi commented Nov 5, 2024 •

edited

Loading