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

Jump to bottom

Fix stderr to output string with no decoration (fix #2063) #2751

Merged
merged 1 commit into from
Jul 23, 2023
Merged

Fix stderr to output string with no decoration (fix #2063) #2751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion src/builtin.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1246,7 +1246,11 @@ static jv f_debug(jq_state *jq, jv input) {
}

static jv f_stderr(jq_state *jq, jv input) {
jv_dumpf(jv_copy(input), stderr, 0);
jq_msg_cb cb;
void *data;
jq_get_stderr_cb(jq, &cb, &data);
if (cb != NULL)
cb(data, jv_copy(input));
return input;
}

Expand Down
15 changes: 15 additions & 0 deletions src/execute.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ struct jq_state {
void *input_cb_data;
jq_msg_cb debug_cb;
void *debug_cb_data;
jq_msg_cb stderr_cb;
void *stderr_cb_data;
};

struct closure {
Expand Down Expand Up @@ -1018,6 +1020,9 @@ jq_state *jq_init(void) {
jq->debug_cb = NULL;
jq->debug_cb_data = NULL;

jq->stderr_cb = NULL;
jq->stderr_cb_data = NULL;

jq->err_cb = default_err_cb;
jq->err_cb_data = stderr;

Expand Down Expand Up @@ -1249,6 +1254,16 @@ void jq_get_debug_cb(jq_state *jq, jq_msg_cb *cb, void **data) {
*data = jq->debug_cb_data;
}

void jq_set_stderr_cb(jq_state *jq, jq_msg_cb cb, void *data) {
jq->stderr_cb = cb;
jq->stderr_cb_data = data;
}

void jq_get_stderr_cb(jq_state *jq, jq_msg_cb *cb, void **data) {
*cb = jq->stderr_cb;
*data = jq->stderr_cb_data;
}

void
jq_halt(jq_state *jq, jv exit_code, jv error_message)
{
Expand Down
3 changes: 2 additions & 1 deletion src/jq.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,10 @@ jv jq_get_error_message(jq_state *);
typedef jv (*jq_input_cb)(jq_state *, void *);
void jq_set_input_cb(jq_state *, jq_input_cb, void *);
void jq_get_input_cb(jq_state *, jq_input_cb *, void **);

void jq_set_debug_cb(jq_state *, jq_msg_cb, void *);
void jq_get_debug_cb(jq_state *, jq_msg_cb *, void **);
void jq_set_stderr_cb(jq_state *, jq_msg_cb, void *);
void jq_get_stderr_cb(jq_state *, jq_msg_cb *, void **);

void jq_set_attrs(jq_state *, jv);
jv jq_get_attrs(jq_state *);
Expand Down
15 changes: 15 additions & 0 deletions src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,18 @@ static void debug_cb(void *data, jv input) {
fprintf(stderr, "\n");
}

static void stderr_cb(void *data, jv input) {
if (jv_get_kind(input) == JV_KIND_STRING) {
int dumpopts = *(int *)data;
priv_fwrite(jv_string_value(input), jv_string_length_bytes(jv_copy(input)),
stderr, dumpopts & JV_PRINT_ISATTY);
} else {
input = jv_dump_string(input, 0);
fprintf(stderr, "%s", jv_string_value(input));
}
jv_free(input);
}

#ifdef WIN32
int umain(int argc, char* argv[]);

Expand Down Expand Up @@ -684,6 +696,9 @@ int main(int argc, char* argv[]) {
// Let jq program call `debug` builtin and have that go somewhere
jq_set_debug_cb(jq, debug_cb, &dumpopts);

// Let jq program call `stderr` builtin and have that go somewhere
jq_set_stderr_cb(jq, stderr_cb, &dumpopts);

if (nfiles == 0)
jq_util_input_add_input(input_state, "-");

Expand Down
10 changes: 10 additions & 0 deletions tests/shtest
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,16 @@ grep "Expected string key after '{', not '\\['" $d/err > /dev/null
echo '{"x":"y",["a","b"]}' | $JQ --stream > /dev/null 2> $d/err || true
grep "Expected string key after ',' in object, not '\\['" $d/err > /dev/null

# debug, stderr
$VALGRIND $Q $JQ -n '"test", {} | debug, stderr' >/dev/null
Copy link
Member

@wader wader Jul 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Should output be compared somehow here?

Copy link
Member

Choose a reason for hiding this comment

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

Unrelated thought: I wonder if i would be nicer if we replaced $VALGRIND $Q $JQ by having jq in path be a wrapper script that does exec valgrind -q /path/to/real/jq/ ...? or do we don't want to not run via valgrind in some cases when it's enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put this line just for valgrind (and I actually found a leak in the first commit), and I heard Nico said we don't care about the output of debug/0 somewhere. I strongly hope that JQ takes care of valgrind automatically but there might some difficulties in testing the standard error output.

Copy link
Member

@wader wader Jul 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Aha valgrind can messes up stdout/err? by "hope that JQ takes care" do you mean it would be nice if $JQ pointed to some path that runs jq with valgrind?

Yes think i read that comment from @nicowilliams also, but even if we make no guarantees about stableness maybe it can make sense to test debug output so that we would notice if it changes by accident? maybe we already do somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

debug/0 should do something with its output. We could have a command-line option to send that to some file, or /dev/null/NUL, and debug/0 can decorate its output or not (but it helps that it does).

I don't really care about the mechanics of how valgrind or other memory debuggers get invoked, as long as a) such debuggers can be optional, b) they can be turned off (valgrind slows dev cycles). If the current thing makes it easy to forget to use valgrind, sure, let's fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if in --run-tests we set a stderr callback, we could have syntax in .test files for checking stderr's output, though I don't think we need that given that a test here in shtest is just as well, and we only need stderr to test it.

$JQ -n -c -j '"hello\nworld", null, [false, 0], {"foo":["bar"]}, "\n" | stderr' >$d/out 2>$d/err
cat > $d/expected <<'EOF'
hello
worldnull[false,0]{"foo":["bar"]}
EOF
cmp $d/out $d/expected
cmp $d/err $d/expected

# --arg, --argjson, $ARGS.named
$VALGRIND $JQ -n -c --arg foo 1 --argjson bar 2 '{$foo, $bar} | ., . == $ARGS.named' > $d/out
printf '{"foo":"1","bar":2}\ntrue\n' > $d/expected
Expand Down