Page MenuHomePhabricator

Create install targets for scan-build-py.
AcceptedPublic

Authored by isthismyaccount on Apr 22 2021, 11:30 PM.

Details

Summary

Create install targets for scan-build-py and include this target in fuchsia distribution.

Also move lib folder to match the distribution layout.

TESTED=local build. Verify scan-build could execute.

Diff Detail

Event Timeline

aabbaabb created this revision.Apr 22 2021, 11:30 PM
aabbaabb requested review of this revision.Apr 22 2021, 11:30 PM
aabbaabb set the repository for this revision to rG LLVM Github Monorepo.
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2021, 11:31 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I don't think this is sufficient. First, we want to be explicit and always list individual files to install, we don't use patterns. Second, since libscanbuild is a library, it shouldn't be installed inside the bin directory. I think those should go either into lib or share. libscanbuild/resources should definitely go into share as is already the case for scan-build. Third, libear is a native component so that one needs to be compiled first.

The python script assumes relative directory while finding things. For example, for resources folder, it uses os.path.join(this_dir, 'resources') in report.py, which means resource need to be in the same dir as report.py. Similarly for the libscanbuild. it assumes the library is always at one level up from bin folder. Installing them to different directories would break the script.

I don't think this is sufficient. First, we want to be explicit and always list individual files to install, we don't use patterns. Second, since libscanbuild is a library, it shouldn't be installed inside the bin directory. I think those should go either into lib or share. libscanbuild/resources should definitely go into share as is already the case for scan-build. Third, libear is a native component so that one needs to be compiled first.

I don't think this is sufficient. First, we want to be explicit and always list individual files to install, we don't use patterns. Second, since libscanbuild is a library, it shouldn't be installed inside the bin directory. I think those should go either into lib or share. libscanbuild/resources should definitely go into share as is already the case for scan-build. Third, libear is a native component so that one needs to be compiled first.

libear is built dynamically at runtime from build_libear function in libear/__init__.py which would be called by libscanbuild/analyze.py. It is not built statically.

The python script assumes relative directory while finding things. For example, for resources folder, it uses os.path.join(this_dir, 'resources') in report.py, which means resource need to be in the same dir as report.py. Similarly for the libscanbuild. it assumes the library is always at one level up from bin folder. Installing them to different directories would break the script.

We could reorganize things to match the final layout, that's the strategy that https://github.com/llvm/llvm-project/tree/main/clang/tools/scan-build uses as well.

libear is built dynamically at runtime from build_libear function in libear/__init__.py which would be called by libscanbuild/analyze.py. It is not built statically.

Could we modify the code to avoid building libear at runtime and instead build it with CMake. Is libear even needed when using compilation database?

The python script assumes relative directory while finding things. For example, for resources folder, it uses os.path.join(this_dir, 'resources') in report.py, which means resource need to be in the same dir as report.py. Similarly for the libscanbuild. it assumes the library is always at one level up from bin folder. Installing them to different directories would break the script.

We could reorganize things to match the final layout, that's the strategy that https://github.com/llvm/llvm-project/tree/main/clang/tools/scan-build uses as well.

libear is built dynamically at runtime from build_libear function in libear/__init__.py which would be called by libscanbuild/analyze.py. It is not built statically.

Could we modify the code to avoid building libear at runtime and instead build it with CMake. Is libear even needed when using compilation database?

If i copy libscanbuild/resources to out/share/, libscanbuild to out/libscanbuild, bin/* to bin, then resources would be in a different layout than the original src. You mean modifying the original source and move libscanbuild/resources out to share/resources and update the code reference?

We are not using libear, we are only using analyze-build not scan-build. For our usage, I could define a target that only copies analyze-build, not scan-build. However, I am not sure whether this is appropriate. On the other hand, building libear statically might require significant change to the code.

The python script assumes relative directory while finding things. For example, for resources folder, it uses os.path.join(this_dir, 'resources') in report.py, which means resource need to be in the same dir as report.py. Similarly for the libscanbuild. it assumes the library is always at one level up from bin folder. Installing them to different directories would break the script.

We could reorganize things to match the final layout, that's the strategy that https://github.com/llvm/llvm-project/tree/main/clang/tools/scan-build uses as well.

libear is built dynamically at runtime from build_libear function in libear/__init__.py which would be called by libscanbuild/analyze.py. It is not built statically.

Could we modify the code to avoid building libear at runtime and instead build it with CMake. Is libear even needed when using compilation database?

If i copy libscanbuild/resources to out/share/, libscanbuild to out/libscanbuild, bin/* to bin, then resources would be in a different layout than the original src. You mean modifying the original source and move libscanbuild/resources out to share/resources and update the code reference?

Do we still keep the source in sync with https://github.com/rizsotto/scan-build? I was under the impression that the two codebases have already started diverging.

We are not using libear, we are only using analyze-build not scan-build. For our usage, I could define a target that only copies analyze-build, not scan-build. However, I am not sure whether this is appropriate. On the other hand, building libear statically might require significant change to the code.

We may consider introducing a CMake option to control whether to include libear or not (which would also control whether to include scan-build or not).

aabbaabb updated this revision to Diff 341352.Apr 28 2021, 4:29 PM
aabbaabb edited the summary of this revision. (Show Details)

The python script assumes relative directory while finding things. For example, for resources folder, it uses os.path.join(this_dir, 'resources') in report.py, which means resource need to be in the same dir as report.py. Similarly for the libscanbuild. it assumes the library is always at one level up from bin folder. Installing them to different directories would break the script.

We could reorganize things to match the final layout, that's the strategy that https://github.com/llvm/llvm-project/tree/main/clang/tools/scan-build uses as well.

libear is built dynamically at runtime from build_libear function in libear/__init__.py which would be called by libscanbuild/analyze.py. It is not built statically.

Could we modify the code to avoid building libear at runtime and instead build it with CMake. Is libear even needed when using compilation database?

If i copy libscanbuild/resources to out/share/, libscanbuild to out/libscanbuild, bin/* to bin, then resources would be in a different layout than the original src. You mean modifying the original source and move libscanbuild/resources out to share/resources and update the code reference?

Do we still keep the source in sync with https://github.com/rizsotto/scan-build? I was under the impression that the two codebases have already started diverging.

We are not using libear, we are only using analyze-build not scan-build. For our usage, I could define a target that only copies analyze-build, not scan-build. However, I am not sure whether this is appropriate. On the other hand, building libear statically might require significant change to the code.

We may consider introducing a CMake option to control whether to include libear or not (which would also control whether to include scan-build or not).

Updated the layout and listed individual files.
It is not trivial to separate out scan-build with analyze-build since all the logic is in one python lib file. Furthermore, libear is compiled with the cc compiler that is supplied as an argument to the tool, so i cannot turn that into a prebuilt without a large refactor to the codebase. I added a comment listing the reason.

wanders added a subscriber: wanders.May 3 2021, 7:12 AM
wanders added inline comments.
clang/tools/scan-build-py/CMakeLists.txt
9

This overwrites the "bin/scan-build" that was installed from the scan-build (no -py) subdirectory.

So with this "scan-build" will suddenly mean the python variant instead of the per variant. That might be fine (?), but can't be a good idea to have that done by first installing the perl scan-build and then overwriting it with the python scan-build. Also the man-page that the perl variant installs is not overwritten so "man scan-build" will show manual page for the perl variant.

aabbaabb updated this revision to Diff 342529.May 3 2021, 1:32 PM

Rename scan-build to scan-build-py during install to prevent overwriting pearl implementation.

aabbaabb marked an inline comment as done.May 3 2021, 1:33 PM
aabbaabb added inline comments.
clang/tools/scan-build-py/CMakeLists.txt
9

Thanks for the comment. I have renamed scan-build to scan-build-py during installation.

aabbaabb marked an inline comment as done.May 3 2021, 1:34 PM
phosek added a comment.May 6 2021, 1:41 PM

Looking at libear/__init__.py, it's basically replicating CMake build to the point where it even emulates CMake template expansion, see https://github.com/llvm/llvm-project/blob/a3a8a1a15b524d91b5308db68e9d293b34cd88dd/clang/tools/scan-build-py/libear/__init__.py#L204. Given that we already use CMake, I think we should just build libear with CMake and ship libear as shared library and then modify https://github.com/llvm/llvm-project/blob/a3a8a1a15b524d91b5308db68e9d293b34cd88dd/clang/tools/scan-build-py/libscanbuild/intercept.py#L113 to use the prebuilt version instead of building it every time. However, given that this change is already pretty large, I'm also fine if we do it in a follow up change.

clang/tools/scan-build-py/CMakeLists.txt
2–8

I'd prefer to install all of these into libexec to match Perl scan-build.

28
29

A property we're trying to keep is that the build layout matches the installation layout, so in addition to installing files, we also need a custom target to copy these files into the build output directory. This is what Perl scan-build does as well, see https://github.com/llvm/llvm-project/blob/a3a8a1a15b524d91b5308db68e9d293b34cd88dd/clang/tools/scan-build/CMakeLists.txt#L41

aabbaabb updated this revision to Diff 343541.May 6 2021, 5:07 PM
aabbaabb marked 3 inline comments as done.

Renamed lib to libexec and add custom rules to copy files to build output.

phosek added inline comments.May 12 2021, 10:30 AM
clang/tools/scan-build-py/CMakeLists.txt
2–8

Shouldn't this list also contain scan-build?

10
22
aabbaabb updated this revision to Diff 344904.May 12 2021, 12:11 PM
aabbaabb marked 3 inline comments as done.

Renamed lib to libexec and add custom rules to copy files to build output.

That rename was a bit too fast I think.
I would expect the libraries (libear & libscanbuild) to be in lib/.
The internal wrappers analyze-c++,analyze-cc,intercept-c++,intercept-cc to be installed in libexec.
And finally analyze-build, intercept-build, scan-build which are the commands user can execute to be installed in bin/

(I have picked up this patch in our internal toolchain as it gives us a way to generate compilation databases out of the box. It works fine here)

aabbaabb updated this revision to Diff 345329.May 13 2021, 6:20 PM

put libs in 'lib' folder, 'analyze-*' and 'scan-*' compiler wrappers to 'libexec', also copy the resources in 'libscanbuild'

Tests:

To install:
DESTDIR=${INSTALL_DIR} ninja install-scan-build-py -j1000

Test at source folder:
PATH=~/llvm-project/build/libexec:~/llvm-project/build/bin:$PATH python -m unittest tests.unit
PATH=~/llvm-project/build/libexec:~/llvm-project/build/bin:$PATH python -m unittest tests.functional.cases

One error in functional test: test_successful_build_on_empty_env
I think this is intended behavior (clang not found?) since the test sets an empty environment for make.

I think this looks good. I can't vouch for the cmake changes.

However there seems like diff is based on something that already has moved parts into libexec. It doesn't apply cleanly here - and buildkite seems to complain too. Make sure you have the complete diff.

phosek accepted this revision.Jun 9 2021, 10:37 AM

LGTM

This revision is now accepted and ready to land.Jun 9 2021, 10:37 AM

Thanks for the reviews. I'll take over this and try to get the merge conflicts resolved.

isthismyaccount commandeered this revision.Jun 10 2021, 4:58 PM
isthismyaccount edited reviewers, added: aabbaabb; removed: isthismyaccount.

Grabbing this revision.

Rebased to hopefully resolve merge issues.