Requirement Document generator tool
Christian Mauderer
christian.mauderer at embedded-brains.de
Thu Feb 6 08:26:10 UTC 2020
Hello Chris,
please let me pick up the discussion again so that we manage to come
to a conclusion. Like Matthias said oflist, we are trying to address
the problem of the missing project overview in a separate thread. But
it would be really relevant to get a result for the python topics.
Again: In my view the discussion has been triggered by the
pre-qualification efforts but it isn't limited to it. Having standards
for how to handle python tools would make adding and maintaining new
tools simpler in any case.
Although we will have some more topics (test approach, static
analysis, documentation style, ...) I'll try to concentrate on the
style issues for now. The other topics are much less relevant for the
existing code.
I already said that in another mail but please let me repeat it again:
My target wouldn't be to reformat existing code. My target is to find
something that would be acceptable for all new code and that is
similar enough that existing code is still readable and maybe can
converted when a change is necessary for some other reason. PEP 8 even
suggests to not reformat code if it predates a coding standard:
https://www.python.org/dev/peps/pep-0008/#a-foolish-consistency-is-the-hobgoblin-of-little-minds
Google style doesn't express it that directly but there are "Parting
Words" too that tell basically the same:
https://google.github.io/styleguide/pyguide.html#4-parting-words
Below I added some answers to the code locations that you specifically
mentioned before.
On 28/01/2020 11:41, Chris Johns wrote:
> 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 <chrisj at rtems.org
>>>> <mailto:chrisj at 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):
How strong do you feel about these changes? Like noted in another mail
it's one of the official Python PEPs (PEP 8) that suggests to have no
spaces here. See:
https://www.python.org/dev/peps/pep-0008/#other-recommendations
>
> 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'
> + ]
I played a bit with yapf: The simplest possibility to have lists that
are formatted that way is to add a "," at the end. So if that
structure looks like follows:
return cfgs + ['objcopy',
'arch',
'vendor',
'board',
'config_name',
'first_stage',
'second_stage',
'kernel_converter',]
yapf would reformat it to the following:
return cfgs + [
'objcopy',
'arch',
'vendor',
'board',
'config_name',
'first_stage',
'second_stage',
'kernel_converter',
]
Which in my view would be quite OK. There are options regarding
indentation if you really would prefer it. From a quick look I've
found about 10 locations in rtems-tools that would need that. Most
likely I missed about half to two thirds of them so it would be more
of 20 to 30.
>
>
>> 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)
Again: How strong do you feel about that? PEP 8 as well as google
seems to allow both. So there is most likely an an option in yapf to
prefer the one or the other.
>
> 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.
I agree that they are not readable. I didn't find an option for yapf
to align them automatically. But there is the "# yapf: disable" for
such cases. It would just mean that this comment would have to be
added to the last bracket:
'_forced_static': ('none', 'none', '')
} # yapf: disable
I have found four locations in rtems-tools where it would be necessary
and a few more where it might could be a good idea. I think there are
some in libbsd.py too. But overall it seems to be a really small
number of locations where such workarounds would be necessary. Again:
Only if we want to reformat existing code which is _not_ recommended
by PEP 8.
Best regards
Christian
>
> 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 at rtems.org <mailto:devel at rtems.org>
>>>> http://lists.rtems.org/mailman/listinfo/devel
>>>>
>>> _______________________________________________
>>> devel mailing list
>>> devel at rtems.org
>>> http://lists.rtems.org/mailman/listinfo/devel
>>>
>>
--
--------------------------------------------
embedded brains GmbH
Herr Christian Mauderer
Dornierstr. 4
D-82178 Puchheim
Germany
email: christian.mauderer at embedded-brains.de
Phone: +49-89-18 94 741 - 18
Fax: +49-89-18 94 741 - 08
PGP: Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
More information about the devel
mailing list