tsaarni / cpp-subprocess

popen() -like C++ library with iostream support for stdio forwarding
MIT License
88 stars 21 forks source link

Example as given in README doesn't work #6

Open deets opened 4 years ago

deets commented 4 years ago

I tried to get the simple ls-example working, but failed. Starting/Waiting/Closing all works, but not getting a buffer.

I had to resort to

https://github.com/deets/brombeerquark/blob/master/cpp/subprocess/src/cpp-subprocess.cpp

(taken from the tests) to get things to work. The command used btw. is not the culprit, I did in fact start with your ls-example. Just had no reason to commit that.

I have no idea why this is not working. I'm running on a raspberry pi with a YOCTO linux, but glibc and GCC 8.3.0.

tsaarni commented 4 years ago

Sorry, I'm not able to reproduce it.

I don't have access to Yocto though, but I tried on Raspbian, by modifying your example with rdbuf()

$ cat main.cpp
#include "subprocess.hpp"
#include <chrono>
#include <thread>

int main(int argc, char *argv[])
{
  using namespace std::chrono_literals;
  while(true)
  {
    std::string buf;
    subprocess::popen cmd("vcgencmd", {"measure_temp"});
    std::cout << cmd.stdout().rdbuf();
    cmd.wait();
    std::this_thread::sleep_for(1s);
  }
  return 0;
}
$ g++ --version
g++ (Raspbian 8.3.0-6+rpi1) 8.3.0
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$ g++ -std=c++17 -Iinclude main.cpp -o main
$ sudo ./main
temp=40.0'C
temp=40.0'C
...
dir-ableton commented 4 years ago

I agree that it is weird, I did try it several times. I could - if you were willing to look a bit deeper - hand you an image to flash, as well as a cross compilation SDK, if that helps. Let me know.

tsaarni commented 4 years ago

I must admit I don't have a clue what could be wrong. Since you have the development environment there already existing, maybe you can try couple of random things:

Maybe the problem could be related to buffering. You could try calling out_stream->rdbuf()->pubsetbuf(0,0) to disable buffering after this line but I don't think that necessarily does anything in this case since it is said "The way to do this is just to turn off the buffering before any I/O operations at all have been done (note that opening counts as an I/O operation)".

Secondly, the call to stdout() returns a reference to out_stream here. I wonder, is there any possibility that it interferes with the iostream object? Maybe you could try to return pointer instead and see if there is any effect.

edit: Thirdly, could it be that call to rdbuf() in relation to the forked & executed program would become timing critical? You could try experiment by adding sleep after launching the program and taking the buffer and consuming it.

dir-ableton commented 4 years ago

Ok, a short update: I was running the following code, and the first of your above suggestions applied:

#include "subprocess.hpp"
#include <chrono>
#include <thread>

int main(int argc, char *argv[])
{
  using namespace std::chrono_literals;
  while(true)
  {
    std::cout << "before\n";
    subprocess::popen cmd("ls", {});
    std::cout << "after\n";
    std::cout << cmd.stdout().rdbuf();
    std::cout << "waiting..\n";
    std::this_thread::sleep_for(1s);
  }
  return 0;
}

No output, as before.

The following is the session:

root@move-dir:~# /tmp/cpp-subprocess &
[1] 900
root@move-dir:~# before
after

root@move-dir:~# 
root@move-dir:~# 
root@move-dir:~# ps |grep ls
  276 root         0 SW   [irq/169-ablspi_]
  901 root         0 Z    [ls]
  902 root         0 Z    [ls]
  903 root         0 Z    [ls]
  904 root         0 Z    [ls]
  905 root         0 Z    [ls]
  906 root         0 Z    [ls]
  907 root         0 Z    [ls]
  908 root         0 Z    [ls]
  909 root         0 Z    [ls]
  911 root      2200 S    grep ls
root@move-dir:~# fg
/tmp/cpp-subprocess
^C
root@move-dir:~# 

So I

To me this indicates that the issue here is somehow todo with meddling with the stdout of the invoking process. Or more general iostreams, because the following example showed the exact same issue:

#include <chrono>
#include <thread>

int main(int argc, char *argv[])
{
  using namespace std::chrono_literals;
  std::ofstream of("/tmp/output");
  while(true)
  {
    of << "before\n";
    subprocess::popen cmd("ls", {});
    of << "after\n";
    of << cmd.stdout().rdbuf();
    of << "waiting..\n";
    of.flush();
    std::this_thread::sleep_for(1s);
  }
  return 0;
}

Adding a std::this_thread::sleep_for(1s); as by your suggestion before trying to capture didn't do anything good, either.

I don't think there is a difference between a reference and a pointer, but I will investigate.

dir-ableton commented 4 years ago

As I feared, changing the reference to a pointer didn't affect the outcome :(

tsaarni commented 4 years ago

Thanks for troubleshooting, and sorry that I could not help.

One more random idea: Does it make any difference if you change std::ios_base::in to std::ios_base::in | std::ios_base::out | std::ios_base::binary on line: https://github.com/tsaarni/cpp-subprocess/blob/2e91c3945df3894648ecdcdbd1c197eedb22e63a/include/subprocess.hpp#L119

tsaarni commented 4 years ago

And one more test:

subprocess::popen cmd("ls", {});
cmd.wait();
std::cout << cmd.stdout().rdbuf();

that is, wait for process to terminate before call to rdbuf().

dir-ableton commented 4 years ago

Neither the wait nor the file mode did change a thing :( It's really weird. It did solve the issue of accumulating processes though. Don't you think it would be a good idea to implicitly invoke wait() in ~popen()?

dir-ableton commented 4 years ago

Ok, one more bit of information: it works by using printf for printing. I don't get the output printed yet, there is probably something more happening when invoking operator<<, but the before/after are coming! I got this idea after staring at the strace log. I wonder if somehow the C++ stream runtime state is corrupted, and why.

tsaarni commented 4 years ago

Could it be somehow distantly related to this https://stackoverflow.com/questions/35261389/stringstream-rdbuf-causing-cout-to-fail, that is: "std::cout is unable to read any characters ... sets failbit on std::cout, and until this state is cleared any further output will fail too". I'm not that familiar with iostreams library to understand this really, and in your case rdbuf() is read only once so it should not be at the end - this still would be unexplained. But it seems consistent with your observation of cout getting in failed state.

Don't you think it would be a good idea to implicitly invoke wait() in ~popen()?

Yeah, I think you are right. There is potential problem though, since wait() will block until the executed command exits, but I guess that may be expected by user, at least in most cases. Other alternatives for preventing zombies would be ignoring SIGCHLD or installing signal handler for it that collects the status by calling wait() but those are something that user should decide on.

dir-ableton commented 4 years ago

Just regarding the last point (will research your first suggestion later): I think the current behavior is the most suprising one. From a RIAA-perspective (and this is how I thought about this as a pattern) it follows for me that the lifetime of the object should govern the lifetime of it's resources. One of the few really bright sides of C++ ;) I agree though that the behavior is not ideal. So from my POV we can do these things:

I'm slightly in favor of killing, because it doesn't make the program hang, and exceptions are a choice that many C++-devs don't want to be forced to use. What do you think?

tsaarni commented 4 years ago

Yes, makes sense to me. Maybe the option between wait and kill should be provided anyways even if kill would be default.

Killing brings up yet another question: should it be SIGKILL or SIGHUP, where latter would give the child an option for graceful termination.

Furthermore, if the process has already terminated earlier kill() would not do anything, so kill would probably anyway imply waitpid(child, nullptr, WNOHANG) too, to cover that specific scenario.

dir-ableton commented 4 years ago

I think only SIGKILL makes sense, as SIGHUP can be ignored, and then we are in the same space as just doing an implicit wait: the program hangs, and the user has to find out why. Regarding the waitpid I have to admit I'm not deep enough in the details, but it sounds as if you are ;)

MLutt commented 4 years ago

Been over a month since the last reply - ran into the same issue and had to resort to using Python for now ( https://gist.github.com/MLutt/4b50cdd1644564307bda0d276e235ab4 )... Any progress on it?

tsaarni commented 4 years ago

I have not had time to work with this, sorry for trouble.

tsaarni commented 4 years ago

@MLutt I was not able to reproduce the problem previously, but for future reference, if you can list information on your environment it might be helpful (e.g. linux distribution, gcc/glibc version...)

MLutt commented 4 years ago

@MLutt I was not able to reproduce the problem previously, but for future reference, if you can list information on your environment it might be helpful (e.g. linux distribution, gcc/glibc version...)

image

Anything else?

tsaarni commented 4 years ago

Linux Mint 19.3 seems to be based on Ubuntu 18.04. I'm able to run following

$ docker run --rm -it ubuntu:18.04
root@ebba422a9ccd:/# apt-get update
root@ebba422a9ccd:/# apt-get install build-essential wget
root@ebba422a9ccd:/# gcc --version
gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
root@ebba422a9ccd:/# wget https://raw.githubusercontent.com/tsaarni/cpp-subprocess/master/include/subprocess.hpp
root@ebba422a9ccd:/# cat > test.cpp <<EOF
#include "subprocess.hpp"

int
main(int argc, char *argv[])
{
    subprocess::popen cmd("ls", {});
    std::cout << cmd.stdout().rdbuf();

    return 0;
}
EOF
root@ebba422a9ccd:/# g++ -o test test.cpp
root@ebba422a9ccd:/# ./test
bin
boot
dev
etc
home
lib
lib64
media
mnt
opt
proc
root
run
sbin
srv
subprocess.hpp
sys
test
test.cpp
tmp
usr
var

but for you rdbuf() returns nothing, like reported earlier in this issue?

oskarirauta commented 8 months ago

@tsaarni

I had issues as well. I fixed it by renaming stdout(), stderr() and stdin() to out(),err() and in(). It seems that stdout, stderr and stdin were reserved.

tsaarni commented 8 months ago

Thanks @oskarirauta, interesting finding!

Do you know how to reproduce it? I did not manage myself. Now I made attempt to just have #define stdout foo and check if the method changes its name but it was not that simple, the pre-processor is aware they are methods in a class and it does not do simple replace. It did mess up the stdout file handle from <cstdio> though :laughing: (checked with compiler option -E to see the pre-processor output). But I guess it does not matter after all. If renaming really is needed for some environments, then it makes sense to rename...

oskarirauta commented 8 months ago

@tsaarni

Sure thing; here goes..

dev@media:~/cpp-subprocess/include# cat test.cpp
#include <iostream>
#include "subprocess.hpp"

int main(int argc, char **argv) {

    subprocess::popen cmd("ls", {});
    std::cout << cmd.stdout().rdbuf();

    return 0;
}
dev@media:~/cpp-subprocess/include# g++ test.cpp
In file included from /usr/include/c++/13.2.0/cstdio:42,
                 from /usr/include/c++/13.2.0/ext/string_conversions.h:45,
                 from /usr/include/c++/13.2.0/bits/basic_string.h:4097,
                 from /usr/include/c++/13.2.0/string:54,
                 from /usr/include/c++/13.2.0/bits/locale_classes.h:40,
                 from /usr/include/c++/13.2.0/bits/ios_base.h:41,
                 from /usr/include/c++/13.2.0/ios:44,
                 from /usr/include/c++/13.2.0/ostream:40,
                 from /usr/include/c++/13.2.0/iostream:41,
                 from test.cpp:1:
test.cpp: In function 'int main(int, char**)':
test.cpp:7:26: error: expected unqualified-id before '(' token
    7 |         std::cout << cmd.stdout().rdbuf();
      |                          ^~~~~~
dev@media:~/cpp-subprocess/include# gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-openwrt-linux-musl/13.2.0/lto-wrapper
Target: x86_64-openwrt-linux-musl
Configured with: /usr/src/openwrt/build_dir/target-x86_64_musl/gcc-13.2.0/configure --target=x86_64-openwrt-linux --host=x86_64-openwrt-linux --build=x86_64-pc-linux-musl --disable-dependency-tracking --program-prefix= --program-suffix= --prefix=/usr --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin --libexecdir=/usr/lib --sysconfdir=/etc --datadir=/usr/share --localstatedir=/var --mandir=/usr/man --infodir=/usr/info --disable-nls 'CXXFLAGS_FOR_TARGET=-g -O2 -D_GLIBCXX_INCLUDE_NEXT_C_HEADERS' --build=x86_64-pc-linux-musl --host=x86_64-openwrt-linux-musl --target=x86_64-openwrt-linux-musl --enable-languages=c,c++ --with-bugurl=https://dev.openwrt.org/ --with-pkgversion='OpenWrt GCC 13.2.0' --enable-shared --disable-__cxa_atexit --with-default-libstdcxx-abi=gcc4-compatible --enable-target-optspace --with-gnu-ld --disable-nls --enable-__cxa_atexit --enable-lto --disable-libsanitizer --disable-libvtv --disable-libcilkrts --disable-libmudflap --disable-libmpx --disable-multilib --disable-libgomp --disable-libquadmath --disable-libssp --disable-decimal-float --disable-libstdcxx-pch --with-host-libstdcxx=-lstdc++ --prefix=/usr --libexecdir=/usr/lib --with-local-prefix=/usr --with-stage1-ldflags=-lstdc++
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 13.2.0 (OpenWrt GCC 13.2.0) 
dev@media:~/cpp-subprocess/include/fixed# uname -a
Linux media 6.1.57 #0 SMP Sat Oct 14 13:51:53 2023 x86_64 Openwrt/Linux
dev@media:~/cpp-subprocess/include# cd fixed
dev@media:~/cpp-subprocess/include/fixed# diff -Naur ../subprocess.hpp subprocess.hpp 
--- ../subprocess.hpp   2024-01-19 16:10:42.054540733 +0200
+++ subprocess.hpp  2024-01-19 16:09:43.152203011 +0200
@@ -65,15 +65,15 @@
         delete err_stream;
     }

-    std::ostream& stdin()  { return *in_stream;  };
+    std::ostream& in()  { return *in_stream;  };

-    std::istream& stdout()
+    std::istream& out()
     {
         if (out_stream == nullptr) throw std::system_error(EBADF, std::system_category());
         return *out_stream;
     };

-    std::istream& stderr() { return *err_stream; };
+    std::istream& err() { return *err_stream; };

     int wait()
     {
dev@media:~/cpp-subprocess/include/fixed# diff -Naur ../test.cpp test.cpp 
--- ../test.cpp 2024-01-19 16:15:05.634887433 +0200
+++ test.cpp    2024-01-19 16:09:51.422531249 +0200
@@ -4,7 +4,7 @@
 int main(int argc, char **argv) {

    subprocess::popen cmd("ls", {});
-   std::cout << cmd.stdout().rdbuf();
+   std::cout << cmd.out().rdbuf();

    return 0;
 }
dev@media:~/cpp-subprocess/include/fixed# g++ test.cpp
dev@media:~/cpp-subprocess/include/fixed# ./a.out
a.out
subprocess.hpp
test.cpp
dev@media:~/cpp-subprocess/include/fixed#
dev@media:~/cpp-subprocess/include/fixed# cat /proc/cpuinfo 
processor   : 0
vendor_id   : GenuineIntel
cpu family  : 6
model       : 142
model name  : Intel(R) Core(TM) i5-10210U CPU @ 1.60GHz
...

Intel system with modified lightweight openwrt os and gcc 13.2.0. Modifications to os include development headers and libraries that allow building normally and some extra tools; for example, I can re-build (and I often do) complete openwrt including it's kernel with the same system. Those changes normally are not included in the stock version.

Only change was that I renamed stdout() -> out(), stderr() -> err and stdin() -> in and it built without issues.

Outside of topic; I kinda forked your work- or at least re-used parts for streams- check it out if curious.. process_cpp - it though builds with c++20 as minimum... There's not much that cpp-subprocess would gain (maybe args vector handling though..) since it's very different. It wasn't a fork in the first place, but I had troubles with streams until I found this and thought that this is nearly perfect for my needs, so I made a combination of sorts. It also does not have this issue, due to it's different approach :wink:

EDIT: added cpuinfo and changed console shots to more readable format by separating each command to it's own section.

tsaarni commented 8 months ago
test.cpp:7:26: error: expected unqualified-id before '(' token
    7 |         std::cout << cmd.stdout().rdbuf();
      |                          ^~~~~~

Oh ok, there it clearly fails because stdout has been replaced with something else - so likely it really is conflict with preprocessor macro. Interesting that my simple attempt to #define stdout foo did not do the same for me ๐Ÿ˜ตโ€๐Ÿ’ซ

I guess that others in this github issue had something else going wrong, since I understood it failed at runtime for others, not at compile time.

Outside of topic; I kinda forked your work- or at least re-used parts for streams- check it out if curious.. process_cpp -

Cool stuff ๐Ÿ‘ Great that this small project helped with your own version!

Just curious, if you want to tell, what do you do with openwrt? I used to compile & use it when it ran on Linksys WRT54G ๐Ÿ˜†

oskarirauta commented 8 months ago

Yes.. I am attempting to make my own version of lcd4linux (lcd2linux for now..) since lcd4linux's development has ended ages ago, I made a new widget(a bargraph widget for monitoring network usage) for it and once I thought that I want to take it one more step further, I noticed that it was impossible; growing widget enough made it un-usable (I was able to use it, but nothing showing on display) - so I started my own, it's working nicely already, has widgets for images and truetype text rendering, and now it's time to make plugins for it; one being "exec", so I needed my own for that.. It's not that much a fork of this project but since I used all the "important pieces", I decided that it's best way to give credit for using your code in it. My version of lcd2linux has only support for graphical displays and probably will not support anything else than dpf/ax206 (hacked photo frame) such as this, since that's a display I have and use - as a attempt of it supporting original like expression engine, I have created my own expr_cpp - for the moment as there are no plugins yet available; it is just displaying a simple switching image looking like this:

anim It's a mock up display's boot screen: 115342rnpi71nxngywzp7w png Ofcourse it cannot be used as animated boot screen though, as animated boot screens are not supported, and I actually have not even had a chance to change that screen, there are some tools.. Somewhere.. Available for older Windows operating systems.. Just made that for fun and as something that I was able to display and animate when I had it first time working in some manner.

For the moment, there is no repository for this project since I am writing it alone and it's quite a big, so pushing frequently would just slow me down. Configuration is very similar to original with some changes, original had a way to add multiple display configurations and multiple layout configurations and then you chose one of them; this only has 1 for display and layout, but on the other hand, timers can have multiple expressions, and something called "actions" with expression as a conditional whether to do the action or not, and layers have "pages"; actions should have at least "change page" and so on. So you can set it to change page for example periodically. Ofcourse, bar graph is going to be there as well, but not yet, since I use exec for it to retrieve the necessary information through a ubus version of network monitoring that I also have made, ubus-bm and haven't yet made a single plugin and wanted to start it from that plugin. Though I have most of time,date, string and calculation related plugins integrated to expression engine as a default provided functions - all plugins that were available with original, are not going to be there since for example, apm seems to be deprecated, and battery monitoring is complex, especially when all my laptops are from Apple and are not running linux, so what I cannot test; will not be there. Also xmms and others will be missing as I don't use them either, though I am not sure if old plugins work either. It is all written in C++ and is fully extensible like original was, just has a way lot to offer due to chance to infinitely extend your own widgets/plugins as it's not limited like original one was, ofcourse, running those widgets/plugins takes more time to complete so one must consider how big he wants his plugin/widget be; original runned in multiple threads, which was quite a good feature, I wasn't able to build the same thing in smart way, so I have just a simple scheduler which runs through timers and widgets, updates them if necessary - one by one and in the end of cycle, if content on screen has changed, it re-draws it to display.

I also plan to have plugins for json parsing and optional built-in ubus client (similar to dbus, but minimal version; only in openwrt), that way, to retrieve some information on openwrt, one wouldn't even need to use exec in some cases..

It supports all the original image formats using the same gd library as original used.. Code is there very similar, except this time, scaling actually works.. This also translates coordinates, so if you decide to rotate, original coordinates rotate along. Up to your configuration if they draw on-screen as well though..

So, a small project, eh..? ๐Ÿ˜‰

Oh, openwrt.. openwrt-shot

My server is running openwrt, I am a contributor to project, I develop it (on my own and for myself, some of my modifications are not worth of making a push request, since they serve mostly myself, such as some targeted kernel modifications); and ofcourse, I am running it on quite a few of these a bit more powered routers; my network has 1 router, and 5 other routers mainly acting as a swithes, main router has i7 and 8 1gb ports 32gb ram, living room runs i5 with 6 ports and 16gm ram(one that I develop), and others are running with i3, 3 ports and 16gb ram. All the time I hear how openwrt is limited for this and that, but it isn't true if you are willing to have some effort to it; yet it isn't running graphical ui with office kind of software; but it can perform for all that I think Linux is good for; and who says it doesn't have a graphical ui? lcd4linux has been there for quite some time, and lcd2 is on-going ;) - though I don't know if it will be available as a official package.. I've had some trouble contributing to project for past few years, so I tend to have a 3 month breaks on my package update cycles..

I like openwrt mostly because it is what linux used to be; today, there are not many like it- alpine linux is like it. Others, like ubuntu.. Come with graphical user interface, office suite, have huge amount of depencies who no one is able to track... If you want to put it in a single word; they are huge. My own bake of openwrt base bistro is a bit more than 100mb and has a whole lot of stuff from the beginning, and after installing development environment, it's still way less than 200mb; my server runs podman to run containers for nginx, caddy and php and is accessible only through a vpn (and host provided vnc). upodman

Even though I use a lot more console, I think LuCi is a great interface to retrieve status on so many things ๐Ÿ˜ƒ

I also had wrt54g(s)'s, and I am sure that I still do(in the attic in a box).. I've been member of community and contributor since those.. Huh, I rambled quite a lot...

tsaarni commented 8 months ago

@oskarirauta That is a cool project, thanks for telling about it! I wish you good times with your project ๐Ÿ˜„

I never used lcd4linux myself but I knew of it, though just from the yellow-green displays. I never before looked at the variety of extensions and other aspects.

Distantly related, I wanted to build e-ink display project with inkplate10 to follow weather, energy consumption (and pรถrssisรคhkรถ) etc but the project got stalled and waits for new inspiration. There my idea was just to have purpose-built web app rendered as bitmap and trying to display that with the micro controller / display.

oskarirauta commented 8 months ago

Yeah, that sounds more like a Arduino related project. There are boards that come with wifi integrated, consider those.. I've tried to stay away from ESP-32, but it might also be a good board for that purpose.