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

Lukas Slebodnik lslebodn at redhat.com
Wed Jun 11 12:46:55 UTC 2014


On (11/06/14 14:52), Nikolai Kondrashov wrote:
>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.
the 1st one.

>
>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, andi
>it is possible that CI might not need some packages the RPM needs.
I don't think so or they are negligible.

>Thus, we would still need to maintain an extra
>package list for CI purposes.
RPM dependencies are subset of CI dependencies.

>
>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.
You do no need to extract dependencies from spec.file. We already have tool for
this purpose. yum-builddep.
If you have problem with yum-builddep you can use rpmbuild as well
    e.g. rpm -q -p --requires rpmbuild/SRPMS/sssd-1.11.92-0.fc20.src.rpm

>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.
I cannot see difference between executing yum install and yum-builddep.
yum-builddep  is  a program which installs the RPMs needed to build the
specified package. It won't execute any any pre(post) scripts from spec file.

>
>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.
RPM dependencies are subset of CI dependencies.

you can install CI dependencies with yum install and sssd dependencies with
yum-builddep

>Considering these points, I would prefer keeping the CI dependency list
>separate.
disagree.
I do not want to mantain the same list of build dependencies on two different
places.

>
>>I am fine with maintaning development packages for other distributions.
>
>I'm not sure what you mean by the above, could you please elaborate?
Sorry, I meant:
I am fine with maintaning build dependencies in CI script
for other distributions (debian).

>
>>>+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?
>
Executing "rm -Rf" is dangerous. At least, I would like to see that
I did mistake with script modification in log file.

>>>+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?
>
no.
I am used to debugging optimized code. The same with valgrind.
It is not so comfortable, but it is possible.
Some bugs are caused by compiler optimisation and would not be catched by CI.
You can use different CFLAGS for different task.

>>>+        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.
>
Ignoring is not a good way for CI. We need to find a viable solution.

>>>+    # 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.
>
The same type of warning can cause false possitives and catch real errors.
Disabling warnings is not correct.

>>>+    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?
>
yes, they are two different things.

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

>>>+    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?
"libbash_test.info" is a copy&paste error from another script.
it is not problem with external. lcov will generate code coverage
for tests: "src/tests". It will slightly increase percentage of code coverage.

>Thank you for the review, Lukas.
Thank you fer CI effort.

LS



More information about the sssd-devel mailing list