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

cli: Add new history command #1667

Merged
merged 4 commits into from
Nov 12, 2024
Merged

cli: Add new history command #1667

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b468938
cli: Add new history command
Nov 5, 2024
4d62b1d
Run clang
AbrarQuazi Nov 5, 2024
4dd91e2
address pr comments
AbrarQuazi Nov 6, 2024
a94ede3
update tagdag function
AbrarQuazi Nov 6, 2024
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
View file
Open in desktop
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 •
edited
Loading

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

wake/src/runtime/database.h

Line 55 in b468938

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:

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)

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
View file
Open in desktop
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
View file
Open in desktop
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
30 changes: 21 additions & 9 deletions tools/wake/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,15 +168,14 @@ 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;
}
}

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 +227,18 @@ 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);
}
}
AbrarQuazi marked this conversation as resolved.
Show resolved Hide resolved

} // namespace

void print_help(const char *argv0) {
Expand Down Expand Up @@ -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
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 +432,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 +651,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