[SSSD] [RFC] Add basic support for CI test execution

Nikolai Kondrashov Nikolai.Kondrashov at redhat.com
Wed Jun 11 11:52:30 UTC 2014


Hi Lukas,

On 06/11/2014 10:52 AM, Lukas Slebodnik wrote:
>> [1] http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/new_private_master_fedora20/214/console
> The host is not accesible. It should not be in commit message.
>
> $ ping sssd-ci.idm.lab.eng.brq.redhat.com
> ping: unknown host sssd-ci.idm.lab.eng.brq.redhat.com

Do you mean it is not accessible from outside Red Hat or from anywhere?
If the former is the problem, then sure, I'll remove it.
Otherwise we should fix that.

Although the fact that this URL won't be accessible after a while anyway as
the storage is scarce in the lab (why can't that be fixed, BTW?) warrants
removing it from the commit message.

> I would like to avoind maintain two different lists with build dependency for
> fedora/rhel based OSes. We already have listed all build dependencies in
> sssd spec file. It acn be used with rhel6, rhel7 and recent versions of fedora.
> You can install build dependencies with command:
>      a) yum-builddep path/to/package.src.rpm
>      b) yum-builddep path/to/package.spec
>
> sssd.spec file can be created from contrib/sssd.spec.in with few replacements.
>      @see contrib/fedora/make_srpm.sh

The RPM dependencies and CI dependencies are not exactly the same thing. CI
needs some packages the RPM doesn't, and it is possible that CI might not need
some packages the RPM needs. Thus, we would still need to maintain an extra
package list for CI purposes.

The process of extracting the dependencies from the .spec file is
non-straightforward, considering we need to do substitutions to make it valid.
Such code would need to track changes to both the Makefile.am and the .spec
file. This would require having separate code for different distributions,
which is also a maintenance burden.

Then, a rule to run yum-builddep cannot be added to sudoers safely as it
basically allows execution of any code in RPM macros, compared to just adding
a rule to run package installation from repos defined by local admin. This
would force users to either enter sudo password often or significantly lower
security.

A package missing from CI dependencies would be immediately obvious as CI
builds would fail and it would be easy to then update the lists.

Considering these points, I would prefer keeping the CI dependency list
separate.

> I am fine with maintaning development packages for other distributions.

I'm not sure what you mean by the above, could you please elaborate?

>> +DISTRO_BRANCH+=-`lsb_release --id | sed -e 's/^[^:]*:\s*\(.*\)$/\L\1\E/'`
>> +DISTRO_BRANCH+=-`lsb_release --release | sed -e 's/^[^:]*:\s*\(.*\)$/\L\1\E/'`
>> +DISTRO_BRANCH+=-
> The executable file lsb_release needn't be installed and it will cause
> warnings.
> sh-4.2$ DISTRO_BRANCH+=-`lsb_release --id | sed -e 's/^[^:]*:\s*\(.*\)$/\L\1\E/'`
> sh: lsb_release: command not found

Yes, perhaps the script should produce an error with a suggestion to install
lsb_release.

>> +    chmod -Rf u+w -- "$@" || true
>> +    rm -Rf -- "$@"
> If rm is called in recursive mode from script it will be good to print command
> to the standard output before execution or run rm in verbose mode.
>      echo  rm -Rf -- "$@"
>      rm -Rf -- "$@"

Why would that be good?

>> +declare -r DEBUG_CFLAGS="-O0 -g3"
> In my opinion, sssd should be built without optimisation only for code
> coverage. Some warnings are not visible without optimisation.

Wouldn't optimization mess with Valgrind traces?
Should we maybe do a separate build just to detect warnings?

>> +        if [ "`xqilla -i \"\$f\" /dev/stdin \
>              Could you use xmllint instead of xqilla.
> We need xmllint for building manual pages. I didn't try it, but here is the 2nd
> link from quick google search.
> http://stackoverflow.com/questions/15461737/how-to-execute-xpath-one-liners-from-shell

Ah, I didn't know xmllint could that. Thank you.
I tested it and will change to xmllint in the next version.

>> +    # Ignored until issues found by Valgrind are fixed
> We have already ticket for this, but it will not be so easy.

Well, I know it won't be easy, that's why I didn't try fixing it by myself :)

> We need to find some compromise if we do not want to ignore valgrind issues
> forever.
>      https://fedorahosted.org/sssd/ticket/1052

Sure, there are various ways to make Valgrind happy, although none of them are
very easy. The important thing is that we would have a way to track it now.
This way, or another we should arrive to zero Valgrind errors.

>> +    # Ignored until issues found by the scanner are fixed
> clang static analyzer has some false positives. I do not think we will fix all
> of them.

Sure. We can cater to Clang in the code, or we can simply disable some of the
checks. The important thing is that we start, and no matter how, we can
achieve zero errors and enforce the check.

>> +    stage scan-check        scan_check "$scan_report_dir" ||
>> +                            true
>> +
> I would prefer to split this function into two parts.
>      * running static analyser
>      * code coverage.
> The compilation with scan-build is slow and it is not necessary for code
> coverage.

Do you mean that someone would prefer running one without the other? Which one
is more important in your opinion? Would you run one of them locally?

I'd personally run Clang more often than the code coverage collection just to
catch coding mistakes.

>> +    stage lcov-merge        lcov --add-tracefile lcov_base.info \
>> +                                 --add-tracefile lcov_check.info \
>> +                                 --output-file lcov.info
> Could you also remove output for external libraries and tests.
>      lcov --remove libbash_test.info "/usr*" ...

I can use "--no-external". What is "libbash_test.info"? What does passing
"/usr*" to lcov do?

Thank you for the review, Lukas.

Sincerely,
Nick



More information about the sssd-devel mailing list