Page MenuHomePhabricator

[ZORG] Add new builder for CTU analysis
AcceptedPublic

Authored by gamesh411 on May 13 2019, 3:12 AM.

Details

Summary

In order to provide CI coverage for CTU related functionality, a new builder
is introduced. The builder uses CodeChecker tool to drive the analysis, and
run the default Clang SA checkers in CTU mode on open source project TMUX.
CodeChecker: https://github.com/Ericsson/codechecker
TMUX: https://github.com/tmux/tmux

Event Timeline

gamesh411 created this revision.May 13 2019, 3:12 AM
gamesh411 retitled this revision from Add new builder for CTU analysis to [ZORG] Add new builder for CTU analysis.May 13 2019, 3:16 AM
gamesh411 added a reviewer: gkistanova.

Friendly ping @gkistanova. What do you think about this modification? Also, should we add our buildslave to the silent (if I remember correctly it is called staging), or the reporting buildmaster? Thanks!

Hello Endre,

Please start from adding a builder to the staging buildbot first. Once it is stable and reliably green, you could move it to the production master.

I have noticed that you do not pass env to any of the custom steps in your factory. Is this intentional?

Please also see my notes inline.

Thanks

Galina

zorg/buildbot/builders/CTUAnalysisBuilder.py
26

Could you elaborate why it is not possible to use the buildbot way of setting build step envars and run the command remotely by ShellCommand? Why do you need to swap a new shell session here?

101

UnifiedTreeBuilder.getCmakeWithNinjaBuildFactory sets the TERM env for you. No need to define TERM and merge, you can just pass env down to UnifiedTreeBuilder.getCmakeWithNinjaBuildFactory as is.

107

The same with extra_configure_args. Just pass it down to UnifiedTreeBuilder.getCmakeWithNinjaBuildFactory as is.

121

Is there a strong reason to have defaults hardcoded as a part of the buildfactory rather than require these being specified as builder params or a properties?

131

Please use RemoveDirectory from buildbot.steps.slave instead. Here and in all similar places in this patch.

@gkistanova Your review is very much appreciated!
I have answered the inline comments, and I am currently applying the suggested fixes. After short testing, I will update this revision.

zorg/buildbot/builders/CTUAnalysisBuilder.py
26

source codechecker/venv/bin/activate is necessary, because CodeChecker uses virtualenv to manage Python dependencies, and while it just sets its own specific environmental variables in turn, in my opinion, it is best to rely on the activate script to do this. Also, it is necessary to use it in this prefix form, because the effects of source-ing in a buildstep are not visible in the following buildsteps.

Setting the PATH using realpath is necessary to have absolute paths instead of relative ones. This introduces a dependency on realpath being available, however prior personal experience shows that using absolute paths increase robustness.

An alternative implementation would be to write these steps in a separate bash script, but that would create a dependency from that utility script. In its current form the whole build process does not have any external dependencies, every utility is downloaded/checkek-out from the relevant sources.

In conclusion, I have found this implementation the most optimal for minimizing dependencies and maximizing robustness.

101

Good point, thanks.

107

Same as above.

121

The more I think about it, the more I agree with you. Extracting these into customizable build parameters.

131

Fixing these right now.

gamesh411 updated this revision to Diff 207779.EditedJul 3 2019, 6:36 AM
  • Remove redundant parameter handling
  • Pass env keyword argument to every ShellCommand step
  • Use RemoveDirectory where applicable
gamesh411 marked 6 inline comments as done.Jul 3 2019, 6:48 AM

I have applied the fixes, which seemed fitting. Also see my reasoning behind the command-prefix implementation inline.

CodeChecker repo url and tag as build properties are already handled, and they override default values (see in lines 110-113 in the updated diff), is there a better way? (like providing default values for these properties on the GUI, which currently stays blank and property names and values must both be filled in).

I am creating a new revision for integrating the buildbot into the infrastructure, and I will contact you with the access credentials when it's done.

Cheers,
Endre

gkistanova requested changes to this revision.Jul 10 2019, 10:53 PM

Hello Endre,

Almost there. :)
There is a couple of minor things to address. Please see my notes inline.

zorg/buildbot/builders/CTUAnalysisBuilder.py
26

Could you add a comment to the source code explaining these, please?

118

Did you miss one rm -rf command, or there is a reason not to use RemoveDirectory here?

This revision now requires changes to proceed.Jul 10 2019, 10:53 PM
gamesh411 updated this revision to Diff 211267.Jul 23 2019, 2:32 AM
  • Add implementation rationale, fix forgotten build-step
gamesh411 marked 7 inline comments as done.Jul 23 2019, 2:33 AM
gamesh411 added inline comments.
zorg/buildbot/builders/CTUAnalysisBuilder.py
118

Thanks for catching this!

This revision is now accepted and ready to land.Jul 23 2019, 1:51 PM