util: RunCommandParseJSON followups

Sjors commented at 1:50 pm on December 10, 2020: member

Followups based on #15382 review comments:

clarify the relation between build-aux/m4 and its upstream (including 3 text only version bumps to make comparison to upstream easier), see confusion in #15382 (review) and earlier incident where we (or more precisely: I) accidentally removed local modifications in an update
avoid some hideous and potentially unsafe Boost:Process internals, see #15382 (review)

build: trivially update autoconf archive and document process

ax_check_compile_flag, ax_check_link_flag and ax_check_preproc_flag only have an updated copyright notice, https link and revision number. Pulling in these updates makes it easier to compare to upstream.

Adding a README to document the fact that as of this commit all files are unmodified compared to upstream.

3d94f85829

util: workaround potentially unsafe call to bp::child

This avoids boost's argument splitter and instead passes commands straight to the system shell. As explained in:
https://github.com/bitcoin/bitcoin/pull/15382#discussion_r465212138

In addition, this commit permits multiline std_out and std_err.

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

a5c05dfe11

Sjors commented at 1:51 pm on December 10, 2020: member

@ryanofsky I’m not entirely sure if a5c05df fixes (all) the problem(s) you pointed at in your earlier review

DrahtBot added the label Docs on Dec 10, 2020

DrahtBot added the label Utils/log/libs on Dec 10, 2020

in build-aux/m4/README.md:5 in a5c05dfe11

0@@ -0,0 +1,8 @@
1+## Autoconf archive
2+
3+All files in this folder are from the [Autoconf Archive](https://www.gnu.org/software/autoconf-archive/)
4+([Github](https://github.com/autoconf-archive/autoconf-archive/tree/master/m4) may be ahead of the site).
5+They are unmodified, but may not be the most recent serial.

fanquake commented at 4:10 am on December 13, 2020:

I don’t think this sentence adds much value, and is guaranteed to have to be modified in future.

Sjors commented at 1:56 pm on December 13, 2020:

I (briefly) found it useful myself when I cloned the most recent archive, copied the files and did a git diff.

in build-aux/m4/README.md:8 in a5c05dfe11

0@@ -0,0 +1,8 @@
1+## Autoconf archive
2+
3+All files in this folder are from the [Autoconf Archive](https://www.gnu.org/software/autoconf-archive/)
4+([Github](https://github.com/autoconf-archive/autoconf-archive/tree/master/m4) may be ahead of the site).
5+They are unmodified, but may not be the most recent serial.
6+
7+Future modifications to these files should be listed here and at the top of each file,
8+to prevent accidentally overriding them.

fanquake commented at 4:11 am on December 13, 2020:

We shouldn’t need to list modifications to the .m4 files in 2 separate places. Any modifications should already be documented by the commits to this directory that introduce them.

Sjors commented at 1:58 pm on December 13, 2020:

We did that before and (at least I) managed to accidentally forget about those changes in a followup. That said, I don’t know if this note in the README can prevent that either. A linter script might.

fanquake commented at 4:16 am on December 13, 2020: member

It looks like > half the CIs currently either fail to compile, or run make check.

3d94f85829604ea2421a455826cbb272c681b14c: We no-longer use “trivial” in commit messages, so you can remove that. However, the commit should just be split up, separating changes to each file, and using a commit message like: macro name update to serial N.

Sjors commented at 1:58 pm on December 13, 2020: member

@fanquake thanks for the review. I’ll split the commits (a bit later).

in src/util/system.cpp:1205 in a5c05dfe11

1199@@ -1200,25 +1200,30 @@ UniValue RunCommandParseJSON(const std::string& str_command, const std::string&
1200 
1201     if (str_command.empty()) return UniValue::VNULL;
1202 
1203-    bp::child c(
1204+    // This is here because boost::process quotes the argument given to
1205+    // system when used in combination with boost::process::shell.
1206+    // https://github.com/boostorg/process/issues/95

luke-jr commented at 4:33 pm on December 14, 2020:

Does bp::child need this workaround too? If not, that seems like a reason to avoid bp::system…

in src/util/system.cpp:1219 in a5c05dfe11

1218     );
1219-    if (!str_std_in.empty()) {
1220-        stdin_stream << str_std_in << std::endl;
1221-    }
1222+#endif
1223+    stdin_stream << str_std_in << std::endl;

luke-jr commented at 4:35 pm on December 14, 2020:

This shouldn’t work: bp::system won’t return until the child process is complete, which means it’s done with stdin…

in src/util/system.cpp:1226 in a5c05dfe11

1233-    if (n_error) throw std::runtime_error(strprintf("RunCommandParseJSON error: process(%s) returned %d: %s\n", str_command, n_error, error));
1234+    if (exit) {
1235+        std::string error{std::istreambuf_iterator<char>(stderr_stream), {}};
1236+        throw std::runtime_error(strprintf("RunCommandParseJSON error: process(%s) returned %d: %s\n", str_command, exit, error));
1237+    }
1238+    std::string result{std::istreambuf_iterator<char>(stdout_stream), {}};

luke-jr commented at 4:36 pm on December 14, 2020:

Depending on stdout/stderr buffer sizes and amount of output, the process may block and not exit until we read the output…

luke-jr changes_requested

luke-jr commented at 4:37 pm on December 14, 2020: member

The actual code changes appear to just introduce bugs…?

Sjors commented at 3:06 pm on February 24, 2021: member

Some of this has been superseeded by changes on master, and the rest I’m not clear what to do. So closing this for now.

Sjors closed this on Feb 24, 2021

Rspigler commented at 4:05 am on June 21, 2021: contributor

Mark as up for grabs? For possible followup to #21935 ?

Rspigler commented at 6:11 pm on June 22, 2021: contributor

Is https://github.com/boostorg/process/issues/95 a pre-req for this?

DrahtBot locked this on Aug 16, 2022

util: RunCommandParseJSON followups #20614