[Pki-devel] [PATCH] 71-3 Patch for ticket 316- Adding pylint to the build process - Revised version of 71-2

Tue Aug 6 05:24:19 UTC 2013

Please find attached a revised version of the patch to add pylint to
dogtag's build process. 
The previous patch was added as part of the compose scripts but the
current one is added to the spec-file.

With this patch, building dogtag requires pylint to be installed on the
machine. If any errors/warnings are reported by pylint, the build fails.

The report can be found along with the build log in the packages folder.

--Abhishek

On Wed, 2013-07-31 at 16:43 -0400, Ade Lee wrote:
> 1. There is a super long line in the compose script.  Please break that
> into multiple lines.
> 
> +   echo -e "If the reported issues are false positives, please mark them ignored  \nin the configuration file '$PKI_PWD/$PKI_DIR/dogtag.pylintrc' \nand build again."
> 
> 2. The logic for determining whether or not to stop the script based on the return value is incorrect.
> 
> +#OUTPUT STATUS CODE
> +#       Pylint should leave with following status code:
> +#           * 0 if everything went fine
> +#           * 1 if a fatal message was issued
> +#           * 2 if an error message was issued
> +#           * 4 if a warning message was issued
> +#           * 8 if a refactor message was issued
> +#           * 16 if a convention message was issued
> +#           * 32 on usage error
> +#
> +#       status 1 to 16 will be bit-ORed so you can know which different categories has been issued by analysing pylint output status code
> +
> +result=0
> +if [ $(($status&1)) -eq 1 ] || [ $(($status&2)) -eq 2 ] || [ $(($status&4)) -eq 4 ]
> +then
> +  result=1
> +fi
> 
> what happens if there is an error and a warning?  Then the return value
> will be 6.  You need to decompose the error status and determine which
> bits were set.
> 
> Ade
> 
> 
> On Mon, 2013-07-29 at 11:56 -0400, Abhishek Koneru wrote:
> > PFA the patch  which addressed all comments mentioned below.
> > Raised a ticket for question number 8.
> > 
> > --Abhishek
> > 
> > On Fri, 2013-07-26 at 16:13 -0400, Abhishek Koneru wrote:
> > > A few questions/ comments:
> > > 
> > > 1. In your build, you create a pylintscan directory within the source
> > > tree under pki.  I would prefer this to take place in the build area so
> > > that it does not clutter up your source tree.  The pylintscan is a build
> > > artifact.
> > > 
> > > -- Sure can be moved. I can delete the folder once the scan is done. since the scan actually takes place before
> > >    the build. (As per answer 2 - can i still keep pylintscan in pki?)
> > > 
> > > 2. Do you need to remove the directory prior to each run?
> > > 
> > > -- Not required. I will remove it in the script. Will just leave the pylint-report.
> > >    Is it ok to put the report in the root directory of the code tree - pki?
> > > 
> > > 3. What parts of the pylintrc are not default? ie. which parts did you
> > > change?
> > > 
> > > -- The disable-ids property. (Will not disable conventions [C] and refactor[R].)
> > > -- Max lines - from 80 to 100 
> > > -- Removed 'string' package from deprecated modules.
> > > -- Allowing variables to be named 'rv'. (property - good-names)
> > > -- Set property - include ids = yes.
> > > 
> > > 4. I noticed that you added JSONEncoder to the pylintrc as a class for
> > > which private members should not be checked.  Does that allow you to
> > > eliminate the comment to # pylint: disable-msg=E0202 in encoder.py?
> > > 
> > > -- Forgot to squash a few final changes made to pylintrc. it was actually not included in pylintrc.
> > > 
> > > 5. Can you explain the rationale for ignoring  W0511 and W0105?
> > >  W0105 (pointless-string-statement):
> > >        String statement has no effect Used when a string is used as a statement (which of course has no effect). This is a particular case of W0104 with its own message so you can easily disable it if you’re   #       using those strings as documentation, instead of comments.
> > >  W0511 (fixme): Used when a warning note as FIXME or XXX is detected.
> > > 
> > > -- Will add the same to the comments for disabled ids.
> > > 
> > > 6. I would prefer to not ignore Convention, refactor etc. and to have
> > > these show up in the pylint reports.  Based on the return code for
> > > pylint, you can have the build fail only if the bits for errors and
> > > warnings are set.
> > > 
> > > -- Can be done. will make the change.
> > > 
> > > 7. The pylint command line seems to have all the python executables
> > > listed -- is this required?  It means having to update this list every
> > > time an executable is added, which is a step that can easily be missed.
> > > Can you just point the scan to the top level directory?  
> > > 
> > >  -- Yes. Did not notice that Endi added __init__.py in all directories. Will change that.
> > > 
> > > 8.  Similarly to the point above, is there a way to scan the source tree
> > > to find all the python code?
> > > 
> > >  -- Yes it can be done. But since the number of source files is very small, i used this approach of copying
> > >     the directories directly instead of scanning all the code.
> > > 
> > > On Mon, 2013-07-22 at 21:32 -0400, Abhishek Koneru wrote:
> > > > Please review the patch which adds a script and also the pylint
> > > > configuration file to the code tree. The script is called in the compose
> > > > script for core packages before the actual packaging is done. If any
> > > > errors or warnings are reported by pylint, the build fails.
> > > > 
> > > > I did not add pylint as part of build-requires in the spec file for
> > > > pki-core, but have put a check in the script to bypass trying to scan if
> > > > pylint is not installed but with a comment stating the same in the log.
> > > > 
> > > > --Abhishek
> > > > _______________________________________________
> > > > Pki-devel mailing list
> > > > Pki-devel at redhat.com
> > > > https://www.redhat.com/mailman/listinfo/pki-devel
> > > 
> > > 
> > > 
> > > _______________________________________________
> > > Pki-devel mailing list
> > > Pki-devel at redhat.com
> > > https://www.redhat.com/mailman/listinfo/pki-devel
> > 
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-akoneru-0071-3-Adding-pylint-scan-to-the-build.patch
Type: text/x-patch
Size: 13116 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20130806/8ac06123/attachment.bin>