Page MenuHomePhabricator

[scan-build-py] use subprocess wrapper
ClosedPublic

Authored by rizsotto.mailinglist on Jan 28 2017, 12:52 AM.

Details

Summary

It's a chunk from D26390

Introduce run_command for subprocess.check_ouput

  • which logs the command execution details
  • has the same behavior cross different python versions

Kills SELinux check, which would prevent the dynamic loader trick when that is enabled. Tested on Fedora it works fine.

Diff Detail

Event Timeline

dcoughlin edited edge metadata.Jan 28 2017, 10:29 AM

Thanks for breaking up the patch!

Does this mean that library-preload interposition works on SELinux?

LGTM with the added comment noting the python version where check_output()'s behavior changed.

tools/scan-build-py/libscanbuild/__init__.py
50

I think it would be good to mention what version of python the behavior changed in. This will allow future maintainers to determine whether this logic can safely be removed when support for earlier python versions is dropped.

tools/scan-build-py/libscanbuild/runner.py
126

This change doesn't seem to be related to the functionality mentioned in the summary. Should it be in a separate commit? LLVM's incremental development policy encourages unrelated changes to be in separate patches/commits. This makes it easier for maintainers to determine which commit caused a regression and revert if it necessary. It also makes it easier (and therefore faster) to review.

dcoughlin accepted this revision.Jan 28 2017, 10:29 AM
This revision is now accepted and ready to land.Jan 28 2017, 10:29 AM

About the SELinux change. If the SELinux setup forbid the library preload, it won't work. But till now I have not met system which was configured like that. (Tested on Fedora 22-25.)

tools/scan-build-py/libscanbuild/__init__.py
50

Good idea, but Python does not work like this. Python packages are back ported to earlier Python versions. So a statement that describe this piece of code needed by version X might be obsolete later. If I can determinate which version requires the decode method, I would have been set up the condition based on the Python version numbers. But I can't...

tools/scan-build-py/libscanbuild/runner.py
126

I agree, that it was also a bugfix too. Next time I will commit those in separate commits.

About the SELinux change. If the SELinux setup forbid the library preload, it won't work. But till now I have not met system which was configured like that. (Tested on Fedora 22-25.)

Is there an easy way to detect at run time whether the SELinux setup forbids library preload?

tools/scan-build-py/libscanbuild/__init__.py
50

I'm not asking you to use a version number to determine whether decoding is needed at run time. Instead, I'm asking you to document the version number *after which either decoding is always needed or it is always not*. That is, what is the version of python after which the instance check is definitely not needed?

This will enable us to remove the instance check when we drop support for Python versions before that version. At that point the question of whether the package has been backported to those older versions is not relevant.

Given how packages work in the python ecosystem, is this version possible to determine?

Is there an easy way to detect at run time whether the SELinux setup forbids library preload?

I'm not aware any query command which does that. We can run a test and see does it produce the desired output or not. That would be very portable approach! (As I learned IBM AIX does have security feature that might prevent preload too. Fortunately enough this setup was not yet targeted AIX build support. :))

tools/scan-build-py/libscanbuild/__init__.py
50

Yes, I got the question to document it. But pydoc does not say a word about the return type of these methods. (Might be that some dependency changed, which cause this.) To give the answer what version do what, I need to test against all python versions. Or we should use a dedicated library which tries to hide these kind of miss alignment between Python versions. (But that would be a dependency out of standard packages.)

To my best knowledge, I did isolate this strange Python feature into a single function. And I wish to document the version things, but I don't know any source which provide such information. And see no guarantees that it will be up to date when deprecation happens.

The old versions returns str, which is a no op. The new version returns byte, which needs to be encoded. The underlying check_output call is deprecated in Python 3.5, but still present in 3.6 and 3.7a.