Code review comment for lp:~stgraber/apport/pidns-support

Revision history for this message
Martin Pitt (pitti) wrote :

Thanks Stéphane! The general logic/approach looks fine to me, it's nice to get this working.

To be honest I'm not too thrilled about introducing a completely new shell wrapper for this. In the "normal" case of a non-containerized desktop system it adds further overhead to a crash (the extra shell call), and the script is very hard to test automatically, even for basic things like syntax errors. Also, it hardcodes the installed path of apport, which doesn't respect the install/prefix options of setup.py and makes it hard to test changes to data/apport in the source tree.

Could we put the "%p != %P → call apport in the container" logic into data/apport itself? With os.execl() this is not any more difficult than in shell, it would make this more accessible to integrating with the existing tests, and completely avoids the hardcoded path.

This would also potentially allow us to go a step further and instead of os.execl() just do os.chroot(), so that the host's "apport" script is used in the containers. data/apport doesn't change that often really, and the .crash format has never been broken in the entire apport history (if it does, we'd need to drop the chroot()ing, and go back to exec'ing). What do you think about that?

If all that is impractical, I'd like the new shell wrapper to have "set -euC", and the hardcoded path be replaced with a macro that setup.py install turns into the destination path.

Thanks!

review: Needs Fixing

« Back to merge proposal