search

alandefreitas / matplotplusplus

Matplot++: A C++ Graphics Library for Data Visualization 📊🗾
https://alandefreitas.github.io/matplotplusplus/
MIT License
4.33k stars 333 forks source link

Fix error reporting of popen and pclose for Windows (issue 427) #428

Closed mikucionisaau closed 1 month ago

mikucionisaau commented 1 month ago

Attempt to fix issue 427

alandefreitas commented 1 month ago

Oh... That's interesting.

mikucionisaau commented 1 month ago

@alandefreitas I think the core issue is that popen is hackish: 1) OS does a lot of strange things behind the scenes, like recovering the child process handle from FILE handle. 2) the previous implementation used some weird (internal?) features I could not figure out how they work. Quite possible that something has changed in the C library, and thus recovering the handle to the child process does not work anymore (or perhaps it has never worked as code did not check the results).

It seems that it is quite common to replace popen with custom implementation, even for Linux.

So I rewrote popen functionality from scratch and wrapped FILE and process handle into C++ classes.

On one hand, one can make C API with a custom data structure (looks ugly, see commit history). On the other hand, one can even wrap those pipe classes into std::istream and std::ostream, expose the rich API and hide the process pipe complexity, but the gnuplot backend wrapper does not need that much.

It would be nice to test this PR on a Mac before merging.

alandefreitas commented 1 month ago

Oh... Thanks! This ended up being quite complex. Nice work. Is it good to go? It looks good to me.

CI would check Apple Clang, but it didn't run for some reason. It seems it needs some maintenance anyway because the latest runs broke in https://github.com/alandefreitas/matplotplusplus/actions. That's expected from time to time. It shouldn't block this PR because it's unrelated to it, though.

mikucionisaau commented 1 month ago

I just fixed error reporting for MSVC, which is very weird. Also tested on MacOS with Intel architecture which has no leak sanitizer and address sanitizer seems broken, so I had to disable all sanitizers. The examples seem to work, but I get lots of noise in the output, not sure whom to blame.

Sorry for the big chunk of code. I think it's ready, but feel free to review and complain :-)

alandefreitas commented 1 month ago

Also tested on MacOS with Intel architecture which has no leak sanitizer and address sanitizer seems broken, so I had to disable all sanitizers. The examples seem to work, but I get lots of noise in the output, not sure whom to blame.

Mmmm... That's unfortunate. Testing MacOS is painful.

Sorry for the big chunk of code.

No problem. That's expected here. This popen/pclose thing can become unexpectedly complex.

The code looks great. Thanks!