util: RunCommandParseJSON followups #20614
Conversation
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.
This avoids boost's argument splitter and instead passes commands straight to the system shell. As explained in: bitcoin#15382 (comment) In addition, this commit permits multiline std_out and std_err. Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
|
@ryanofsky I'm not entirely sure if a5c05df fixes (all) the problem(s) you pointed at in your earlier review |
There was a problem hiding this comment.
It looks like > half the CIs currently either fail to compile, or run make check.
3d94f85: 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.
|
|
||
| All files in this folder are from the [Autoconf Archive](https://www.gnu.org/software/autoconf-archive/) | ||
| ([Github](https://github.com/autoconf-archive/autoconf-archive/tree/master/m4) may be ahead of the site). | ||
| They are unmodified, but may not be the most recent serial. |
There was a problem hiding this comment.
I don't think this sentence adds much value, and is guaranteed to have to be modified in future.
There was a problem hiding this comment.
I (briefly) found it useful myself when I cloned the most recent archive, copied the files and did a git diff.
| They are unmodified, but may not be the most recent serial. | ||
|
|
||
| Future modifications to these files should be listed here and at the top of each file, | ||
| to prevent accidentally overriding them. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 thanks for the review. I'll split the commits (a bit later). |
| // This is here because boost::process quotes the argument given to | ||
| // system when used in combination with boost::process::shell. | ||
| // https://github.com/boostorg/process/issues/95 |
There was a problem hiding this comment.
Does bp::child need this workaround too? If not, that seems like a reason to avoid bp::system...
| stdin_stream << str_std_in << std::endl; | ||
| } | ||
| #endif | ||
| stdin_stream << str_std_in << std::endl; |
There was a problem hiding this comment.
This shouldn't work: bp::system won't return until the child process is complete, which means it's done with stdin...
| if (exit) { | ||
| std::string error{std::istreambuf_iterator<char>(stderr_stream), {}}; | ||
| throw std::runtime_error(strprintf("RunCommandParseJSON error: process(%s) returned %d: %s\n", str_command, exit, error)); | ||
| } | ||
| std::string result{std::istreambuf_iterator<char>(stdout_stream), {}}; |
There was a problem hiding this comment.
Depending on stdout/stderr buffer sizes and amount of output, the process may block and not exit until we read the output...
|
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. |
|
Mark as up for grabs? For possible followup to #21935 ? |
|
Is boostorg/process#95 a pre-req for this? |
Followups based on #15382 review comments:
build-aux/m4and its upstream (including 3 text only version bumps to make comparison to upstream easier), see confusion in util: add RunCommandParseJSON #15382 (comment) and earlier incident where we (or more precisely: I) accidentally removed local modifications in an update