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

cli: Add new history command #1667

Merged
merged 4 commits into from
Nov 12, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/runtime/database.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ struct Database::detail {
sqlite3_stmt *tag_job;
sqlite3_stmt *get_tags;
sqlite3_stmt *get_all_tags;
sqlite3_stmt *get_runs_table;
sqlite3_stmt *get_edges;
sqlite3_stmt *get_file_dependency;
sqlite3_stmt *get_output_files;
Expand Down Expand Up @@ -125,6 +126,7 @@ struct Database::detail {
tag_job(0),
get_tags(0),
get_all_tags(0),
get_runs_table(0),
get_edges(0),
get_file_dependency(0),
get_interleaved_output(0) {}
Expand Down Expand Up @@ -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";
Copy link
Contributor Author

@AbrarQuazi AbrarQuazi Nov 5, 2024

Choose a reason for hiding this comment

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

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

struct Time {

AbrarQuazi marked this conversation as resolved.
Show resolved Hide resolved
const char *sql_get_edges =
"select distinct user.job_id as user, used.job_id as used"
" from filetree user, filetree used"
Expand Down Expand Up @@ -470,6 +473,7 @@ std::string Database::open(bool wait, bool memory, bool tty) {
PREPARE(sql_tag_job, tag_job);
PREPARE(sql_get_tags, get_tags);
PREPARE(sql_get_all_tags, get_all_tags);
PREPARE(sql_get_runs_table, get_runs_table);
PREPARE(sql_get_edges, get_edges);
PREPARE(sql_get_file_dependency, get_file_dependency);
PREPARE(sql_get_output_files, get_output_files);
Expand Down Expand Up @@ -528,6 +532,7 @@ void Database::close() {
FINALIZE(tag_job);
FINALIZE(get_tags);
FINALIZE(get_all_tags);
FINALIZE(get_runs_table);
FINALIZE(get_edges);
FINALIZE(get_file_dependency);
FINALIZE(get_output_files);
Expand Down Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

... 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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return get_all_file_dependencies(this, imp->get_file_dependency);
, but realized that is unnecessary for get_runs)

std::vector<RunReflection> out;
db->begin_txn();
while (sqlite3_step(query) == SQLITE_ROW) {
RunReflection run;
run.id = sqlite3_column_int(query, 0);
run.time = Time(sqlite3_column_int64(query, 1));
run.cmdline = rip_column(query, 2);
out.emplace_back(run);
}
finish_stmt("Could not retrieve runs", query, db->imp->debugdb);
db->end_txn();
return out;
}

std::vector<RunReflection> Database::get_runs() const {
return get_all_runs(this, imp->get_runs_table);
}

std::vector<std::pair<std::string, int>> Database::get_interleaved_output(long job_id) const {
std::vector<std::pair<std::string, int>> out;

Expand Down
11 changes: 11 additions & 0 deletions src/runtime/database.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,15 @@ struct Time {
std::string as_string() const;
};

struct RunReflection {
int id;
colinschmidt marked this conversation as resolved.
Show resolved Hide resolved
Time time;
std::string cmdline;
RunReflection() = default;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

RunReflection(int id_, int64_t time_, std::string cmdline_)
: id(id_), time(time_), cmdline(cmdline_) {}
};

struct JobReflection {
long job;
bool stale;
Expand Down Expand Up @@ -169,6 +178,8 @@ struct Database {
std::vector<JobEdge> get_edges();
std::vector<JobTag> get_tags();

std::vector<RunReflection> get_runs() const;
AbrarQuazi marked this conversation as resolved.
Show resolved Hide resolved

std::vector<FileDependency> get_file_dependencies() const;

std::vector<std::pair<std::string, int>> get_interleaved_output(long job_id) const;
Expand Down
3 changes: 3 additions & 0 deletions tools/wake/cli_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ struct CommandLineOptions {
int profileh;
bool last_use;
bool last_exe;
bool history;
bool lsp;
bool failed;
bool script;
Expand Down Expand Up @@ -133,6 +134,7 @@ struct CommandLineOptions {
{'l', "last", GOPT_ARGUMENT_FORBIDDEN},
{0, "last-used", GOPT_ARGUMENT_FORBIDDEN},
{0, "last-executed", GOPT_ARGUMENT_FORBIDDEN},
{0, "history", GOPT_ARGUMENT_FORBIDDEN},
{0, "lsp", GOPT_ARGUMENT_FORBIDDEN},
{'f', "failed", GOPT_ARGUMENT_FORBIDDEN},
{'s', "script", GOPT_ARGUMENT_FORBIDDEN},
Expand Down Expand Up @@ -192,6 +194,7 @@ struct CommandLineOptions {
profileh = arg(options, "profile-heap")->count;
last_use = arg(options, "last")->count || arg(options, "last-used")->count;
last_exe = arg(options, "last-executed")->count;
history = arg(options, "history")->count;
lsp = arg(options, "lsp")->count;
failed = arg(options, "failed")->count;
script = arg(options, "script")->count;
Expand Down
34 changes: 25 additions & 9 deletions tools/wake/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -168,15 +168,16 @@ void hide_internal_jobs(std::vector<std::vector<std::string>> &out) {
out.push_back({"tags NOT LIKE '%<d>inspect.visibility=hidden<d>%'", "tags IS NULL"});
}

void inspect_database(const CommandLineOptions &clo, Database &db, const std::string &wake_cwd) {
// tagdag is technically a db inspection, but its very different from the
// rest, just handle it and exit.
AbrarQuazi marked this conversation as resolved.
Show resolved Hide resolved
if (clo.tagdag) {
JAST json = create_tagdag(db, clo.tagdag);
std::cout << json << std::endl;
return;
void query_runs(Database &db)
{
const auto runs = db.get_runs();
for (const auto run : runs)
{
std::cout << run.time.as_string() << " " << run.cmdline << std::endl;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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?

}
}

void query_jobs(const CommandLineOptions &clo, Database &db){
std::vector<std::vector<std::string>> collect_ands = {};
std::vector<std::vector<std::string>> collect_input_ands = {};
std::vector<std::vector<std::string>> collect_output_ands = {};
Expand Down Expand Up @@ -228,6 +229,20 @@ void inspect_database(const CommandLineOptions &clo, Database &db, const std::st
describe(matching_jobs, get_describe_policy(clo), db);
}

void inspect_database(const CommandLineOptions &clo, Database &db) {
if (clo.tagdag) {
JAST json = create_tagdag(db, clo.tagdag);
std::cout << json << std::endl;
AbrarQuazi marked this conversation as resolved.
Show resolved Hide resolved
return;
}
else if (clo.history) {
query_runs(db);
}
else {
query_jobs(clo,db);
}
}

} // namespace

void print_help(const char *argv0) {
Expand Down Expand Up @@ -268,6 +283,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
Copy link
Contributor

Choose a reason for hiding this comment

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

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"

<< " --failed -f Capture jobs which failed last build" << std::endl
<< " --tag KEY=VAL Capture jobs which are tagged, matching KEY and VAL globs" << std::endl
<< " --canceled Capture jobs which were canceled in the last build" << std::endl
Expand Down Expand Up @@ -420,7 +436,7 @@ int main(int argc, char **argv) {
bool is_db_inspect_capture = !clo.job_ids.empty() || !clo.output_files.empty() ||
!clo.input_files.empty() || !clo.labels.empty() ||
!clo.tags.empty() || clo.last_use || clo.last_exe || clo.failed ||
clo.tagdag || clo.canceled;
clo.tagdag || clo.canceled || clo.history;

// DescribePolicy::human() is the default and doesn't have a flag.
// DescribePolicy::debug() is overloaded and can't be marked as a db flag
Expand Down Expand Up @@ -639,7 +655,7 @@ int main(int argc, char **argv) {
}

if (is_db_inspection) {
inspect_database(clo, db, wake_cwd);
Copy link
Contributor

Choose a reason for hiding this comment

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

This argument was just dead code already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it was already dead code

inspect_database(clo, db);
return 0;
}

Expand Down
Loading