Skip to content

Conversation

joshua-zivkovic
Copy link
Contributor

We wanted to have programmatic access to the data presented in systemd-analyze's plot's SVG output, so this PR adds the ability to output plot data as JSON or table format. This is controlled by using the --JSON= and --table command line options.

Note:
The show_table function in src/analyze/analyze-plot.c appears a few times throughout the project so it might be worth re-factoring it into shared/.

@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Dec 20, 2022
@joshua-zivkovic joshua-zivkovic changed the title Add JSON and table output to systemd-analyze's plot systemd-analyze: Add JSON and table output to systemd-analyze's plot Dec 21, 2022
Copy link
Member

@poettering poettering left a comment

Choose a reason for hiding this comment

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

looks pretty good. some minor comments

@poettering poettering added analyze reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Jan 6, 2023
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Jan 11, 2023
@poettering
Copy link
Member

btw, we generally do line breaks at 109ch, as .editorconfig, .vimrc, .dir-locals.el all encode. you break lines more aggressively than that.

@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Jan 11, 2023
@joshua-zivkovic
Copy link
Contributor Author

btw, we generally do line breaks at 109ch, as .editorconfig, .vimrc, .dir-locals.el all encode. you break lines more aggressively than that.

Would you mind giving an example of this? I've been following surrounding code for how to deal with formatting on the most part.

@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Jan 11, 2023
@poettering poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels Jan 11, 2023
@bluca
Copy link
Member

bluca commented Jan 11, 2023

needs a rebase

Copy link
Member

@yuwata yuwata left a comment

Choose a reason for hiding this comment

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

Please add brief tests for the new settings in test/units/testsuite-65.sh.

const BootTimes* boot,
const char* pretty_times) {
int m = 1, y = 0;
UnitTimes* u;
Copy link
Member

Choose a reason for hiding this comment

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

Please drop it here, and declare in each for loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The last for loop relies on using u as it was previously changed by the for loop before.

@@ -229,7 +216,7 @@ int verb_plot(int argc, char *argv[], void *userdata) {
if (boot->kernel_done_time > 0)
m++;

for (u = times; u->has_data; u++) {
for (u = (UnitTimes*)times; u->has_data; u++) {
Copy link
Member

Choose a reason for hiding this comment

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

for (UnitTimes *u = times; u->has_data; u++) {

@yuwata yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Jan 13, 2023
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Jan 13, 2023
@poettering
Copy link
Member

lgtm. but the ci didn't like it, but it looks like that is the CI's fault, some network issue.

could you rebase and push again, to trigger this again?

@poettering poettering added needs-rebase good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyze good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed
Development

Successfully merging this pull request may close these issues.

4 participants