On 28/1/20 8:14 pm, Christian Mauderer wrote: > On 28/01/2020 01:42, Chris Johns wrote: >> On 28/1/20 10:48 am, Joel Sherrill wrote: >>> On Mon, Jan 27, 2020 at 4:18 PM Chris Johns <chr...@rtems.org >>> <mailto:chr...@rtems.org>> wrote: >>> >>> On 24/1/20 9:57 pm, Jose Valdez wrote: >>> > In fact these tools target the pre-qualified project. >>> >>> Do you see this as different to the RTEMS project? >>> >>> > Since it was Sebastian who suggested to create this set of python >>> tools, >>> >>> I think Sebastian is wanting a smooth path for these tools into the >>> project. >>> >>> > I think the idea was to standardize the use of python not only for >>> this >>> project, but also for other python written code in RTEMS community. >>> This has >>> the advantages that every written python code is standard, but has the >>> drawbacks: >>> > >>> > -> old written code would need to be adapted to the standards. >>> >>> How different to the proposed coding standard is the existing code? Why >>> not base >>> the coding standard on what exists in the code base? >>> >>> This is a very important question. >>> >>> Have you evaluated the size of the task to update the existing code? >>> How would >>> get such changes for the rtems-tools and the RSB be tested and >>> integrated back >>> into the project? This apporach seems like a huge review task for me. >>> >>> It could be or it may turn out that there isn't much changed. Without >>> someone >>> running the reformatter and reporting, we won't know. >> >> Running a reformatter may give you an idea of the scale however I have some >> concerns as Python's logic is based on indent levels and a missed level >> changes >> the logic. I am not sure how you know such a tool is safe to use unless you >> review all the changes. > > To get some statistics for that I tried using yapf on the rtems-tools > repo. I excluded everything with "patch-gdb-python", "tftpy" and > "asciidoc" in the name. You can see the yapf call in the commit message.
The doxygen is from waf. Thomas knows how to write python. > Google style: > https://gist.githubusercontent.com/c-mauderer/18746eaa8f3a077adddfe35c1f7b5194/raw/f8202a53ca043d23732d79ab217071e975dc35ad/0001-FIXME-Format-in-Google-style.patch > > PEP8 Style: > https://gist.githubusercontent.com/c-mauderer/18746eaa8f3a077adddfe35c1f7b5194/raw/c97c77762f65041d9244bc8bc64b90ec698735bf/0001-FIXME-Reformat-using-PEP8.patch > > In both cases it's about 3000 lines removed and 3700 lines added. > > Without having a look at each line: Most seems to be just the normal > formatting stuff. Some lines are too long. Some spaces missing around > operators or spaces where they shouldn't be. And then there is removal of spaces on default args ... - def section_macro_map(self, section, nesting_level = 0): + def section_macro_map(self, section, nesting_level=0): I prefer spaces as it is consistent with other uses of spaces around operators. > Some lists written with > more or less line breaks. Yes to make them readable. This is on purpose. I do not agree with this .. - return cfgs + ['objcopy', - 'arch', - 'vendor', - 'board', - 'config_name', - 'first_stage', - 'second_stage', - 'kernel_converter'] + return cfgs + [ + 'objcopy', 'arch', 'vendor', 'board', 'config_name', 'first_stage', + 'second_stage', 'kernel_converter' + ] > Some extra line breaks added between functions. Yes, this seems to be some that happens. No spaces when setting an argument and then these spaces between functions and methods. > But I also noted a problem for python 3 compatibility: There are some > tab to space conversions. As far as I know python 3 throws at least a > warning or maybe an error if it finds tabs. Yes this happens and there are some cases in the code still. I do not like this ... - argsp.add_argument('--net-boot-file', - help = 'network boot file (default: %(default)r).', - type = str, default = 'rtems.img') - argsp.add_argument('--net-boot-fdt', - help = 'network boot load a fdt file (default: %(default)r).', - type = str, default = None) + argsp.add_argument( + '--net-boot-file', + help='network boot file (default: %(default)r).', + type=str, + default='rtems.img') + argsp.add_argument( + '--net-boot-fdt', + help='network boot load a fdt file (default: %(default)r).', + type=str, + default=None) This is a blocker for me ... - '_ncpus': ('none', 'none', ncpus), - '_os': ('none', 'none', 'darwin'), - '_host': ('triplet', 'required', uname[4] + '-apple-darwin' + uname[2]), - '_host_vendor': ('none', 'none', 'apple'), - '_host_os': ('none', 'none', 'darwin'), - '_host_cpu': ('none', 'none', uname[4]), - '_host_alias': ('none', 'none', '%{nil}'), - '_host_arch': ('none', 'none', uname[4]), - '_host_prefix': ('dir', 'optional', '%{_usr}'), - '_usr': ('dir', 'optional', '/usr/local'), - '_var': ('dir', 'optional', '/usr/local/var'), - '__ldconfig': ('exe', 'none', ''), - '__xz': ('exe', 'required', '%{_usr}/bin/xz'), - 'with_zlib': ('none', 'none', '--with-zlib=no'), - '_forced_static': ('none', 'none', '') - } + '_ncpus': ('none', 'none', ncpus), + '_os': ('none', 'none', 'darwin'), + '_host': ('triplet', 'required', + uname[4] + '-apple-darwin' + uname[2]), + '_host_vendor': ('none', 'none', 'apple'), + '_host_os': ('none', 'none', 'darwin'), + '_host_cpu': ('none', 'none', uname[4]), + '_host_alias': ('none', 'none', '%{nil}'), + '_host_arch': ('none', 'none', uname[4]), + '_host_prefix': ('dir', 'optional', '%{_usr}'), + '_usr': ('dir', 'optional', '/usr/local'), + '_var': ('dir', 'optional', '/usr/local/var'), + '__ldconfig': ('exe', 'none', ''), + '__xz': ('exe', 'required', '%{_usr}/bin/xz'), + 'with_zlib': ('none', 'none', '--with-zlib=no'), + '_forced_static': ('none', 'none', '') + } These cannot be touched. It would make the dicts really hard to maintain. Chris > >> >>> I tend to think it is worth knowing if this is a monster or a mouse before >>> making >>> a decision. >> >> Yes this is important and also if it is a monster which specific rules make >> it >> so? It maybe most of the rules are fine. >> >> If these rules are important to the qual effort I hope they see the value in >> find these answers for us. >> >>> > Another option could be leave it as it is and only do this for new >>> written >>> code. >>> >>> It would be confusing to any new user to the code to have code written >>> to a >>> standard and code that is not? If you edit the old code is it to the new >>> standard? If you edit an old file do you need to update the whole file? >>> >>> If we accept a standard, then it is all or nothing. I'm going to sound like >>> a >>> cranky old man but we have said things like this before and regretted it >>> every time. Consistency is critical. >> >> If you are a cranky old man then that must make me one as well. Ah the youth >> of >> today ... :) >> >>> Quick run of sloccount for a baseline >>> >>> + rtems-tools - >>> Totals grouped by language (dominant language first): >>> ansic: 47237 (49.86%) >>> cpp: 25837 (27.27%) >>> python: 21227 (22.40%) >>> sh: 442 (0.47%) >> >> Nice start. A more accurate report on this code would mean removing the >> imported >> pieces. >> >>> + rtems-source-builder/source-builder - >>> SLOC Directory SLOC-by-Language (Sorted) >>> 14314 sb python=14169,sh=145 >>> 65 top_dir sh=65 >>> 0 config (none) >> >> There are the .cfg and .bset files. >> >>> 0 patches (none) >>> >>> So we have about 35K SLOC or Python by that. >>> >>> No idea how the new standard versus the old looks. I thought Python had a >>> consistent >>> style but I could be very wrong. :( >> >> I am not sure, I have never looked. My python skills have developed producing >> these tools and this code. I know doc comments are missing. >> >>> > -> at some point some tools need to be upgraded (ex: python 3.7 will >>> become unusable in 2030 Operating systems). >>> >>> I am not sure how this relates. Yes it will need to update however we >>> need to >>> support python2 for user facing tools for a while yet. A lot of what we >>> do and >>> how we work is historically driven. >>> >>> CentOS 8 was just released in October. None of the big organization users I >>> see are using it yet. >>> >>> We need to make a LTS release with 5 on Python 2 as a minimum. I feel >>> strongly >>> about that. >> >> And I suspect longer. >> >>> As long as the tools are written in a python agnostic manner, the version >>> won't >>> matter. >> >> Yes I agree, we should aim for a middle ground and avoid hitting any of >> issues >> that can arise. >> >>> We need some test cases for the tools to verify them >> >> Yes. >> >> Chris >> >>> > I hope soon to formalize our suggestion to you and then you may >>> review it >>> (and propose changes if you find appropriate). >>> >>> I suggest working in the open and with us will be more beneficial in the >>> long term. >>> >>> >>> +1 I can't agree strongly enough. >>> >>> >>> >>> Note, I am assuming the remainder of the email was Christian's. The >>> quoting from >>> your email client made it difficult to tell. >>> >>> Chris >>> _______________________________________________ >>> devel mailing list >>> devel@rtems.org <mailto:devel@rtems.org> >>> http://lists.rtems.org/mailman/listinfo/devel >>> >> _______________________________________________ >> devel mailing list >> devel@rtems.org >> http://lists.rtems.org/mailman/listinfo/devel >> > _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel