Python handling Bash/CMake compilation errors

[WetzelVictor]

This PR is a first fix designed to catch compilation errors when using the C++ feature on PyPHS.

---

Principle

It relies on a temporary file, stderr, created in the same folder as run.sh.

When running a C++ based simulation, PyPHS execute a shell script that is the interface between Python and the CMake compiler. Every step is logged in stderr. After the compilation step, Python check for the log:

stderr: end

if this line doesn't appear in the stderr file, python raise a RuntimeError, and the last line of the file, which contains the last step that caused the error.

---

Implementation

Shell script

pyphs/numerics/cpp/simu2cpp.py

• the set -e command affects the behavior of the script: if an error is encountered, bash stops the execution of the script.
• The potentially previous stderr file is removed (if fi control structure)
• Every step is logged in this file: echo "stderr: step n - doing stuff" > stderr
• Confirmation step: inserting stderr: end at the end of the file

Error checking

pyphs/numerics/simulations/simulation.py

• Python reads the file line by line
• Removes the unnecessary characters
• Check if stderr: end is in the list
• Raise an error if not, with the last line of the stderr

---

Drawbacks

This implementation is not the best, but it was the one that required the least amount of time. Some ideas:

• Is the RuntimeError the correct exception to raise?
• Use the actual stdout or stderr created by the execution of bash file. I tried to work with that, but couldn't figure it out.
• I am not sure about the flexibility of this method

[codecov-io]

Codecov Report

Merging #102 into master will decrease coverage by 0.19%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master     #102     +/-   ##
=========================================
- Coverage   82.54%   82.34%   -0.2%     
=========================================
  Files         162      162             
  Lines        7540     7558     +18     
=========================================
  Hits         6224     6224             
- Misses       1316     1334     +18

Impacted Files	Coverage Δ
pyphs/numerics/simulations/simulation.py	62.89% <0%> (-5.58%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16b6f45...50def5d. Read the comment docs.

[FabricioS]

Hi all,
I admit I can understand the purpose of the shell script during the development process, but I am more reluctant about this generated script once the debug phase is over as it mixes build and execution of the simulation code, has no error checking, etc..
We can gain advantage from the subprocess module that has much to offer. For instance, this code:

import os
import subprocess as sp

project_name = 'dampedosc'
kwsp = {
    "cwd": os.path.join(project_name, project_name),
    "stderr": sp.PIPE,
    "check": True,
    "shell": True,
    "universal_newlines": True
}
with open(os.path.join(project_name, project_name, "build.log"), 'w') as fid:
    kwsp['stdout'] = fid
    p = sp.run('cmake . -Bbuild', **kwsp)
    p = sp.run('cmake --build build -- -j3', **kwsp)

kwsp['stdout'] = sp.PIPE
p = sp.run('./bin/%s' % project_name, **kwsp)
print(p.stdout)

raise an error if an error occurs during the build (cmake returning a non zero error code) or the simulation. In this code, the output (stdout and stderr) of the build are logged in the build.log file while the output of the simulation appears in the python interpreter. Many possibilities are available and they greatly simplifies the workflow with respect to this pull request.
Best regards

[WetzelVictor]

Hi,

This implementation seems to be a more robust and flexible solution, that takes advantage of everything the subprocess has to offer.

It will also, in my own opinion, simplify the code inside PyPHS and eliminate a "blind" interface between python and cmake.

[WetzelVictor]

Hi everyone,

I took @FabricioS example code, and implemented it in the _process_cpp() method, inside simulation.py. It catches compilations error, as excepted. (commits
b26d034 and 3fa0d93).

It needs to be tested on Windows. It seems that the option shell has a different behavior. It might be necessary to add a condition like:

if sys.platform.startswith('win'):
    sp_config["shell"] = True
else:
    sp_config["shell"] = False 

Thoughts

• I was wondering if it is useful to print the build log file at the end of the execution? Or setting it as an option?
• Also, what do you think about removing or renaming the old simulation exec file before recompiling? It could help avoiding using an old cpp implementation.

What are your insights on this?

Best regards,

[FabricioS]

This Pull Request seems to integrate things for the other pull request (#100).
Is this intended ?
See also inline comments about how to explicitly report the build errors to users

[FabricioS]

Be careful with indentation. This two-lines change can be reverted

[FabricioS]

You can even be more explicit in the keys of the cmd dictionary. For example:
Bbuild -> build_tree
build -> compile_src

[FabricioS]

You may wrap these two build steps to catch errors, and add a mention to the log in the build.log file. Something like:

        with open(os.path.join(sp_config["cwd"], "build.log"), 'w') as fid:
            sp_config['stdout'] = fid
            try:
                p = subprocess.run(cmd['Bbuild'], **sp_config)
                p = subprocess.run(cmd['build'], **sp_config)
            except subprocess.CalledProcessError as err:
                print('Error while building simulation executable. See %s for details' % fid.name)
                print(p.stdout)
                print(p.stderr)
                raise subprocess.CalledProcessError(err)

[FabricioS]

Regarding this comment:

it needs to be tested on Windows. It seems that the option shell has a different behavior. It might be necessary to add a condition like:

if sys.platform.startswith('win'):
sp_config["shell"] = True
else:
sp_config["shell"] = False

Can you point where the difference is explained in the subprocess docs ? I only see a discussion about shell here : https://docs.python.org/3.5/library/subprocess.html#frequently-used-arguments.
Your code seems to come from this SO question but there is no real explanation.

Some points of interest:

• there is no security hazard here as we do not execute user-defined commands, only secure hard-coded ones.
• the cmake commands are the same for posix and windows (according to CMake docs)
• shell=True allows to pass a single string for the full command line to run, which is not mandatory but convenient
• as far as I understand, shell=True would also consider user's environment variables which may be handy.

[FabricioS]

Regarding

I was wondering if it is useful to print the build log file at the end of the execution? Or setting it as an option?

We may print the build.log for high values of the verbose variables (set in config.py). In relation with this question and the verbose variable, the logging module would also be useful.

Also, what do you think about removing or renaming the old simulation exec file before recompiling? It could help avoiding using an old cpp implementation.

If build errors are now catched correctly, I don't see the point of removing old executables.

[WetzelVictor]

Hi there,

Regarding old changes from PR #100

I've cleaned this PR from any other modifications. The only file concerned now is the simulation.py file

About the Shell boolean option

The SO post is the only one where it seems to be an issue. But this option seems really useful for further developments. Until further tests, lets leave it that way.

Print the build log

I've set the threshold verbose value to 3, but I don't have a clear idea of the value it should have.

Multiple commits

Sorry for the multiple commits. I had a few issues with my text editor, and the fact that I hard reseted my master branch which left some unresolved conflicts.

About this PR

On my computer, with the particular example shown in the first post of this PR, all errors are tracked down :

• Cmake calls
• Binary execution
• File path errors

The code seems a little bit long, but it is more because of the formatting than actual complexity.

As always, if you have any insights on this PR, feel free to review!

Best regards,

[FabricioS]

In fact it is a lot of commits for a final pretty simple change.
As an exercice for your git skills, you could also squash all the commits into a single one which you pull in a new branch. This would make the tree cleaner with a single commit for a simple change.