<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jan 28, 2020, 5:22 AM Christian Mauderer <<a href="mailto:christian.mauderer@embedded-brains.de">christian.mauderer@embedded-brains.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 28/01/2020 11:41, Chris Johns wrote:<br>
> <br>
> <br>
> On 28/1/20 8:14 pm, Christian Mauderer wrote:<br>
>> On 28/01/2020 01:42, Chris Johns wrote:<br>
>>> On 28/1/20 10:48 am, Joel Sherrill wrote:<br>
>>>> On Mon, Jan 27, 2020 at 4:18 PM Chris Johns <<a href="mailto:chrisj@rtems.org" target="_blank" rel="noreferrer">chrisj@rtems.org</a><br>
>>>> <mailto:<a href="mailto:chrisj@rtems.org" target="_blank" rel="noreferrer">chrisj@rtems.org</a>>> wrote:<br>
>>>><br>
>>>> On 24/1/20 9:57 pm, Jose Valdez wrote:<br>
>>>> > In fact these tools target the pre-qualified project.<br>
>>>><br>
>>>> Do you see this as different to the RTEMS project?<br>
>>>><br>
>>>> > Since it was Sebastian who suggested to create this set of python tools,<br>
>>>><br>
>>>> I think Sebastian is wanting a smooth path for these tools into the project.<br>
>>>><br>
>>>> > I think the idea was to standardize the use of python not only for this<br>
>>>> project, but also for other python written code in RTEMS community. This has<br>
>>>> the advantages that every written python code is standard, but has the<br>
>>>> drawbacks:<br>
>>>> ><br>
>>>> > -> old written code would need to be adapted to the standards.<br>
>>>><br>
>>>> How different to the proposed coding standard is the existing code? Why not base<br>
>>>> the coding standard on what exists in the code base?<br>
>>>><br>
>>>> This is a very important question.<br>
>>>><br>
>>>> Have you evaluated the size of the task to update the existing code? How would<br>
>>>> get such changes for the rtems-tools and the RSB be tested and integrated back<br>
>>>> into the project? This apporach seems like a huge review task for me.<br>
>>>><br>
>>>> It could be or it may turn out that there isn't much changed. Without someone<br>
>>>> running the reformatter and reporting, we won't know.<br>
>>><br>
>>> Running a reformatter may give you an idea of the scale however I have some<br>
>>> concerns as Python's logic is based on indent levels and a missed level changes<br>
>>> the logic. I am not sure how you know such a tool is safe to use unless you<br>
>>> review all the changes.<br>
>><br>
>> To get some statistics for that I tried using yapf on the rtems-tools<br>
>> repo. I excluded everything with "patch-gdb-python", "tftpy" and<br>
>> "asciidoc" in the name. You can see the yapf call in the commit message.<br>
> <br>
> The doxygen is from waf. Thomas knows how to write python.<br>
<br>
OK. I missed that one. The patches were just a test to get some<br>
orientation about what it would mean.<br>
<br>
Please also note: I don't want to doubt the python knowledge of anyone.<br>
<br>
> <br>
>> Google style:<br>
>> <a href="https://gist.githubusercontent.com/c-mauderer/18746eaa8f3a077adddfe35c1f7b5194/raw/f8202a53ca043d23732d79ab217071e975dc35ad/0001-FIXME-Format-in-Google-style.patch" rel="noreferrer noreferrer" target="_blank">https://gist.githubusercontent.com/c-mauderer/18746eaa8f3a077adddfe35c1f7b5194/raw/f8202a53ca043d23732d79ab217071e975dc35ad/0001-FIXME-Format-in-Google-style.patch</a><br>
>><br>
>> PEP8 Style:<br>
>> <a href="https://gist.githubusercontent.com/c-mauderer/18746eaa8f3a077adddfe35c1f7b5194/raw/c97c77762f65041d9244bc8bc64b90ec698735bf/0001-FIXME-Reformat-using-PEP8.patch" rel="noreferrer noreferrer" target="_blank">https://gist.githubusercontent.com/c-mauderer/18746eaa8f3a077adddfe35c1f7b5194/raw/c97c77762f65041d9244bc8bc64b90ec698735bf/0001-FIXME-Reformat-using-PEP8.patch</a><br>
>><br>
>> In both cases it's about 3000 lines removed and 3700 lines added.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Is it possible to make changes we think are ok and in both styles? That would start to whittle down the style issues </div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
>><br>
>> Without having a look at each line: Most seems to be just the normal<br>
>> formatting stuff. Some lines are too long. Some spaces missing around<br>
>> operators or spaces where they shouldn't be.<br>
> <br>
> And then there is removal of spaces on default args ...<br>
> <br>
> - def section_macro_map(self, section, nesting_level = 0):<br>
> + def section_macro_map(self, section, nesting_level=0):<br>
> <br>
> I prefer spaces as it is consistent with other uses of spaces around operators.<br>
<br>
In my view these are detail style discussions. Same is true for most of<br>
your notes further below (although I agree with some of your opinions).<br>
But I didn't wanted to trigger any of these. My intention was to give<br>
some impression about how big such changes would be.<br>
<br>
I know that there is quite some python code already in RTEMS. And I'm<br>
also sure that there are already quite some different python styles in<br>
RTEMS. At least as many as programmers touching python code. But<br>
currently the python code is still few enough that it would be possible<br>
to get a uniform style. And pre-qualification or not: I'm really<br>
convinced that a fixed style would be a good thing for the code base in<br>
general.<br>
<br>
I'm quite indifferent which style we use but do you really think that it<br>
is a good idea to roll our own RTEMS-Python-style instead of using one<br>
of the big ones (like PEP8, Google or some other that might is a better<br>
fit for existing code). Rolling our own would lead to:<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">In spite of your points below, taking one of those with a few exceptions isn't a big deal. We have lots of people who have written C code for RTEMS and only a few who have written any Python.</div><div dir="auto"><br></div><div dir="auto">One thing from the pre-qualification effort that you have forgotten is that we are supposed to document our decisions and rationale. If we say we use Python style X with the following changes and that is supported by style checking and formatting tools with the following arguments, then that's it. If someone asks later, we point to the documentation.</div><div dir="auto"><br></div><div dir="auto">That's what a more formal process is all about. Being able to justify decisions later and bring new people on board. A new person can ask why but we don't have to change.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
1. Long discussions about what is THE RIGHT STYLE (already started in<br>
this mail).<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Just looking at the examples, it looks like some long lines get split and in other cases, it combined neatly formatted lines to make something too long and unmanageable. What rules are acting here?</div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
2. A lot of problems that there is no tool that formats or checks<br>
whether code is in THE RIGHT STYLE.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">This is an unproven assertion. Do the tools take arguments to tune points?</div><div dir="auto"><br></div><div dir="auto">I think we need to see which base style we agree with more. Bulk says the two styles start with same handicap. But if style X has one rule we hate that style Y does not, then maybe it is easy.</div><div dir="auto"><br></div><div dir="auto">If both styles agree and we don't have a style checker timing option, then MAYBE we suck it up.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
3. More discussions later because some other developer thinks that THE<br>
RIGHT STYLE is wrong.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">This is answered by documenting the decision, knowing the tooling, and shutting down the discussion by pointing them to our rules and rationale. </div><div dir="auto"><br></div><div dir="auto">Anyone can volunteer but that doesn't give the privilege of causing churn.</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> <br>
>> Some lists written with<br>
>> more or less line breaks.<br>
> <br>
> Yes to make them readable. This is on purpose. I do not agree with this ..<br>
> <br>
> - return cfgs + ['objcopy',<br>
> - 'arch',<br>
> - 'vendor',<br>
> - 'board',<br>
> - 'config_name',<br>
> - 'first_stage',<br>
> - 'second_stage',<br>
> - 'kernel_converter']<br>
> + return cfgs + [<br>
> + 'objcopy', 'arch', 'vendor', 'board', 'config_name', 'first_stage',<br>
> + 'second_stage', 'kernel_converter'<br>
> + ]<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">+1</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> <br>
>> Some extra line breaks added between functions.<br>
> <br>
> Yes, this seems to be some that happens. No spaces when setting an argument and<br>
> then these spaces between functions and methods.<br>
> <br>
>> But I also noted a problem for python 3 compatibility: There are some<br>
>> tab to space conversions. As far as I know python 3 throws at least a<br>
>> warning or maybe an error if it finds tabs.<br>
> <br>
> Yes this happens and there are some cases in the code still.<br>
> <br>
> I do not like this ...<br>
> <br>
> - argsp.add_argument('--net-boot-file',<br>
> - help = 'network boot file (default: %(default)r).',<br>
> - type = str, default = 'rtems.img')<br>
> - argsp.add_argument('--net-boot-fdt',<br>
> - help = 'network boot load a fdt file (default:<br>
> %(default)r).',<br>
> - type = str, default = None)<br>
> + argsp.add_argument(<br>
> + '--net-boot-file',<br>
> + help='network boot file (default: %(default)r).',<br>
> + type=str,<br>
> + default='rtems.img')<br>
> + argsp.add_argument(<br>
> + '--net-boot-fdt',<br>
> + help='network boot load a fdt file (default: %(default)r).',<br>
> + type=str,<br>
> + default=None)<br>
> <br>
> This is a blocker for me ...<br>
> <br>
> - '_ncpus': ('none', 'none', ncpus),<br>
> - '_os': ('none', 'none', 'darwin'),<br>
> - '_host': ('triplet', 'required', uname[4] + '-apple-darwin' +<br>
> uname[2]),<br>
> - '_host_vendor': ('none', 'none', 'apple'),<br>
> - '_host_os': ('none', 'none', 'darwin'),<br>
> - '_host_cpu': ('none', 'none', uname[4]),<br>
> - '_host_alias': ('none', 'none', '%{nil}'),<br>
> - '_host_arch': ('none', 'none', uname[4]),<br>
> - '_host_prefix': ('dir', 'optional', '%{_usr}'),<br>
> - '_usr': ('dir', 'optional', '/usr/local'),<br>
> - '_var': ('dir', 'optional', '/usr/local/var'),<br>
> - '__ldconfig': ('exe', 'none', ''),<br>
> - '__xz': ('exe', 'required', '%{_usr}/bin/xz'),<br>
> - 'with_zlib': ('none', 'none', '--with-zlib=no'),<br>
> - '_forced_static': ('none', 'none', '')<br>
> - }<br>
> + '_ncpus': ('none', 'none', ncpus),<br>
> + '_os': ('none', 'none', 'darwin'),<br>
> + '_host': ('triplet', 'required',<br>
> + uname[4] + '-apple-darwin' + uname[2]),<br>
> + '_host_vendor': ('none', 'none', 'apple'),<br>
> + '_host_os': ('none', 'none', 'darwin'),<br>
> + '_host_cpu': ('none', 'none', uname[4]),<br>
> + '_host_alias': ('none', 'none', '%{nil}'),<br>
> + '_host_arch': ('none', 'none', uname[4]),<br>
> + '_host_prefix': ('dir', 'optional', '%{_usr}'),<br>
> + '_usr': ('dir', 'optional', '/usr/local'),<br>
> + '_var': ('dir', 'optional', '/usr/local/var'),<br>
> + '__ldconfig': ('exe', 'none', ''),<br>
> + '__xz': ('exe', 'required', '%{_usr}/bin/xz'),<br>
> + 'with_zlib': ('none', 'none', '--with-zlib=no'),<br>
> + '_forced_static': ('none', 'none', '')<br>
> + }<br>
> <br>
> These cannot be touched. It would make the dicts really hard to maintain.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">What rule breaks the Darwin line? At least that's what I think I see.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> <br>
> Chris<br>
> <br>
>><br>
>>><br>
>>>> I tend to think it is worth knowing if this is a monster or a mouse before making<br>
>>>> a decision. <br>
>>><br>
>>> Yes this is important and also if it is a monster which specific rules make it<br>
>>> so? It maybe most of the rules are fine.<br>
>>><br>
>>> If these rules are important to the qual effort I hope they see the value in<br>
>>> find these answers for us.<br>
>>><br>
>>>> > Another option could be leave it as it is and only do this for new written<br>
>>>> code.<br>
>>>><br>
>>>> It would be confusing to any new user to the code to have code written to a<br>
>>>> standard and code that is not? If you edit the old code is it to the new<br>
>>>> standard? If you edit an old file do you need to update the whole file?<br>
>>>><br>
>>>> If we accept a standard, then it is all or nothing. I'm going to sound like a <br>
>>>> cranky old man but we have said things like this before and regretted it <br>
>>>> every time. Consistency is critical.<br>
>>><br>
>>> If you are a cranky old man then that must make me one as well. Ah the youth of<br>
>>> today ... :)<br>
>>><br>
>>>> Quick run of sloccount for a baseline<br>
>>>><br>
>>>> + rtems-tools - <br>
>>>> Totals grouped by language (dominant language first):<br>
>>>> ansic: 47237 (49.86%)<br>
>>>> cpp: 25837 (27.27%)<br>
>>>> python: 21227 (22.40%)<br>
>>>> sh: 442 (0.47%)<br>
>>><br>
>>> Nice start. A more accurate report on this code would mean removing the imported<br>
>>> pieces.<br>
>>><br>
>>>> + rtems-source-builder/source-builder - <br>
>>>> SLOC Directory SLOC-by-Language (Sorted)<br>
>>>> 14314 sb python=14169,sh=145<br>
>>>> 65 top_dir sh=65<br>
>>>> 0 config (none)<br>
>>><br>
>>> There are the .cfg and .bset files.<br>
>>><br>
>>>> 0 patches (none)<br>
>>>><br>
>>>> So we have about 35K SLOC or Python by that.<br>
>>>><br>
>>>> No idea how the new standard versus the old looks. I thought Python had a consistent<br>
>>>> style but I could be very wrong. :(<br>
>>><br>
>>> I am not sure, I have never looked. My python skills have developed producing<br>
>>> these tools and this code. I know doc comments are missing.<br>
>>><br>
>>>> > -> at some point some tools need to be upgraded (ex: python 3.7 will<br>
>>>> become unusable in 2030 Operating systems).<br>
>>>><br>
>>>> I am not sure how this relates. Yes it will need to update however we need to<br>
>>>> support python2 for user facing tools for a while yet. A lot of what we do and<br>
>>>> how we work is historically driven.<br>
>>>><br>
>>>> CentOS 8 was just released in October. None of the big organization users I<br>
>>>> see are using it yet. <br>
>>>><br>
>>>> We need to make a LTS release with 5 on Python 2 as a minimum. I feel strongly<br>
>>>> about that.<br>
>>><br>
>>> And I suspect longer.<br>
>>><br>
>>>> As long as the tools are written in a python agnostic manner, the version won't<br>
>>>> matter.<br>
>>><br>
>>> Yes I agree, we should aim for a middle ground and avoid hitting any of issues<br>
>>> that can arise.<br>
>>><br>
>>>> We need some test cases for the tools to verify them<br>
>>><br>
>>> Yes.<br>
>>><br>
>>> Chris<br>
>>><br>
>>>> > I hope soon to formalize our suggestion to you and then you may review it<br>
>>>> (and propose changes if you find appropriate).<br>
>>>><br>
>>>> I suggest working in the open and with us will be more beneficial in the<br>
>>>> long term.<br>
>>>><br>
>>>><br>
>>>> +1 I can't agree strongly enough. <br>
>>>><br>
>>>><br>
>>>><br>
>>>> Note, I am assuming the remainder of the email was Christian's. The quoting from<br>
>>>> your email client made it difficult to tell.<br>
>>>><br>
>>>> Chris<br>
>>>> _______________________________________________<br>
>>>> devel mailing list<br>
>>>> <a href="mailto:devel@rtems.org" target="_blank" rel="noreferrer">devel@rtems.org</a> <mailto:<a href="mailto:devel@rtems.org" target="_blank" rel="noreferrer">devel@rtems.org</a>><br>
>>>> <a href="http://lists.rtems.org/mailman/listinfo/devel" rel="noreferrer noreferrer" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a><br>
>>>><br>
>>> _______________________________________________<br>
>>> devel mailing list<br>
>>> <a href="mailto:devel@rtems.org" target="_blank" rel="noreferrer">devel@rtems.org</a><br>
>>> <a href="http://lists.rtems.org/mailman/listinfo/devel" rel="noreferrer noreferrer" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a><br>
>>><br>
>><br>
<br>
-- <br>
--------------------------------------------<br>
embedded brains GmbH<br>
Herr Christian Mauderer<br>
Dornierstr. 4<br>
D-82178 Puchheim<br>
Germany<br>
email: <a href="mailto:christian.mauderer@embedded-brains.de" target="_blank" rel="noreferrer">christian.mauderer@embedded-brains.de</a><br>
Phone: +49-89-18 94 741 - 18<br>
Fax: +49-89-18 94 741 - 08<br>
PGP: Public key available on request.<br>
<br>
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.<br>
</blockquote></div></div></div>