[Freeipa-devel] [PATCH] 14 Run lint during each build

[Rob Crittenden]

Dmitri Pal wrote:
> On 04/27/2011 12:33 PM, Adam Young wrote:
>> On 04/27/2011 10:24 AM, Martin Kosek wrote:
>>> On Wed, 2011-04-27 at 09:56 -0400, Adam Young wrote:
>>>> On 04/27/2011 09:34 AM, Dmitri Pal wrote:
>>>>> On 04/27/2011 08:13 AM, Jan Cholasta wrote:
>>>>>> On 27.4.2011 13:17, Martin Kosek wrote:
>>>>>>> On Wed, 2011-04-27 at 12:40 +0200, Jan Cholasta wrote:
>>>>>>>> On 26.4.2011 18:14, Martin Kosek wrote:
>>>>>>>>> On Tue, 2011-04-26 at 13:42 +0200, Jan Cholasta wrote:
>>>>>>>>>> Automatically run the lint script during make
>>>>>>>>>> rpms|client-rpms|srpms.
>>>>>>>>>>
>>>>>>>>> NACK until ticket 1184 is resolved and pushed. Currently,
>>>>>>>>> pylint check
>>>>>>>>> fails when optional python packages (like python-rhsm) are not
>>>>>>>>> installed
>>>>>>>>> on the machine. We should be able to build IPA without those
>>>>>>>>> packages
>>>>>>>>> installed.
>>>>>>>> I think printing a note asking the developer to kindly install the
>>>>>>>> missing packages would be sufficient. AFAIK there are currently
>>>>>>>> only 2
>>>>>>>> optional packages: python-rhsm and python-krbV. python-krbV is
>>>>>>>> optional
>>>>>>>> only for the client part of IPA, so you most likely have it already
>>>>>>>> installed and installing python-rhsm is not really much of a chore.
>>>>>>>> That
>>>>>>>> way all of the code would always be checked and the lint script
>>>>>>>> would be
>>>>>>>> free of the unnecessary complexity of handling missing packages.
>>>>>>> I don't think this is a right approach. When the package is optional
>>>>>>> (currently it may be python-rhsm and python-krbV only, but there
>>>>>>> may be
>>>>>>> others in the future) I shouldn't be obliged to install them in
>>>>>>> order to
>>>>>>> build IPA.
>>>>>> You shouldn't be obliged to install them as a user. As a developer,
>>>>>> you should be ready for all kinds of crazy stuff IMHO.
>>>>>>
>>>>>>> When somebody develops something related with the optional
>>>>>>> package he has them installed and the lint will check the
>>>>>>> relevant code
>>>>>>> too.
>>>>>> All of the code goes to the package, so it all should be checked
>>>>>> during the build.
>>>>>>
>>>>>> Imagine situation like this: You change something in module A,
>>>>>> accidentally breaking functionality that module B depends on.
>>>>>> Module A
>>>>>> is checked and no error is found (because it's the kind of issue that
>>>>>> exhibits itself only in certain conditions). Module B isn't checked,
>>>>>> because it also depends on a not-installed optional package. If it
>>>>>> was
>>>>>> checked, it would report an error that would lead you to the error in
>>>>>> module A. But everything looks fine, so the build succeeds, even when
>>>>>> the error is there.
>>>>>>
>>>>>> The situation might be improbable, but IMO the code should be checked
>>>>>> in the same ecosystem every time anyway, because weird stuff could
>>>>>> happen if it wasn't.
>>>>>>
>>>>>>> It is not that big deal, I just think it would be an annoyance for
>>>>>>> developers. But maybe there is a different opinion.
>>>>>> I know we developers are lazy folk, but this is a matter of writing
>>>>>> one simple command, just one time.
>>>>>>
>>>>>>> Martin
>>>>>>>
>>>>> How about a compromise?
>>>>> By default everything is expected to be installed.
>>>>> But there is a command line switch that allows to skip modules you
>>>>> want
>>>>> to skip. You provide the switch and the list of the modules to skip
>>>>> and
>>>>> build will validate only modules that are not in the list.
>>>>>
>>>>> Will something like this work?
>>>>>
>>>> Actually, make the command line switch just means that a  Lint failure
>>>> doesn't stop the build.  That way, by default the build will fail
>>>> unless
>>>> everything is there and checked, but there is a way to move forward for
>>>> building with a subset of packages.
>>> Yes, I think we will can settle with a compromise. My only concern was
>>> not to force the developers to install unnecessary packages for build.
>>>
>>> I would suggest that the build (or "make lint") succeeds without those
>>> optional packages installed, but a warning is printed out that some
>>> packages are missing and not all the code is checked.
>>>
>>> Then it is a developers responsibility to handle this and wouldn't be
>>> force to install those packages for his test builds etc.
>>
>> How about instead it fails bny default, but prints the message "to
>> suppress the lint check stopping the build, run make
>> --no-fail-on-lint"  so that skipping lint is a deliberate decision?
>
>
> Yes this is the approach I prefer.

This is for developers so I'm fine being as harsh as possible. If pieces 
are missing then yes, we should kill the build with extreme prejudice.

We can handle the client target with an argument passed to make-lint to 
only check certain directories of the source tree.

Otherwise I don't think requiring developers to install an extra package 
or two is too onerous. If someone adds support for other distros and 
that makes it difficult then I would ease my stance.

If our goal here is to prevent broken code to get into the build then 
lets do that. If it is painful for developers then we're on the right track.

I would propose that we put it in its strictest mode possible to start 
with and we can see how that works and back off as necessary.

rob