On 06/11/2014 03:46 PM, Lukas Slebodnik wrote:
On (11/06/14 14:52), Nikolai Kondrashov wrote:
> On 06/11/2014 10:52 AM, Lukas Slebodnik wrote:
>> 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.
Even so, we need to have a list of CI-specific dependencies in CI scripts
anyway.
> 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
Apart from complicating the process further by requiring to build the source
package first, this doesn't make much difference, as it would still expand RPM
macros.
> 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.
It will expand macros, which can include Lua scripts, and those can do
basically anything. See
http://www.rpm.org/wiki/PackagerDocs/RpmLua
Try putting %{lua: os.execute('echo "Hello, world!"')} into a spec file
and
running "sudo yum-builddep" on it.
> 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
Sure, but this will be more complicated than just installing them 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.
It is basically a tradeoff of putting a package name in one more place every
time you add a dependency, vs. spending some time on writing and maintaining
more complicated and fragile code, plus having to either enter the sudo
password or lower security, all this without helping other distributions.
I'm not convinced that the latter is better, sorry.
>> 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).
Ah, understood, thank you.
>> 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.
This function is for internal use by CI and the latter is responsible for
correct invocation strictly operating on CI-produced files and directories.
It would be a waste of everyone's attention and screen space to output
debugging information for the code they didn't write every time they run it.
If somebody changes the CI code and would like to see such output they can
always add "-v", or an "echo" temporarily.
>>> +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.
I don't have much experience reading Valgrind traces for code with
optimization enabled, so I can't say if this is justified. Perhaps we can
enable optimization for the start and then see how it goes. I would like to
hear opinions from others as well.
> 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.
I agree and I'm not saying we should ignore it always. This ignoring is to
enable CI earlier, so we can get at least *some* tests to run. And having CI
running these tests (even though they're ignored) will make fixing the
failures easier, than without the CI. If only for the ability to trigger
background runs on various distros at once. Once we have the issues fixed we
will enforce the check.
>>> + # 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.
Yes, but it can be the last resort. Say, in case the analyzer is buggy, or the
warning is caused by the code that is out of our control. And then there are
the times when business dictates moving the real fix to a later time.
This was just a counterargument to "I do not think we will fix all
of them." Some tests are still better than none.
>>> + 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.
So, when would you like to run each of them, what would be your usage pattern?
>>> + 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.
My idea for including these was to allow seeing which test code actually
executes. If having this is less usefull than not having, then I can exclude
them.
Nick