Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on package ordering

["Laszlo Ersek" <lersek@redhat.com>]

On 06/03/20 13:44, Leif Lindholm wrote:
> On Tue, Jun 02, 2020 at 18:20:26 +0200, Laszlo Ersek wrote:
>> On 06/02/20 16:20, Leif Lindholm wrote:
>>> On Tue, Jun 02, 2020 at 15:29:55 +0200, Laszlo Ersek wrote:
>>>> I have not been aware of the header name collision scenario (nor that
>>>> the [Packages] ordering was supposed to work around such issues).
>>>
>>> Nor had I...
>>>
>>>> I work strictly with edk2 proper, where a name collision like this can
>>>> be detected, and so should be prevented. (Insert yet another argument
>>>> why keeping platform code outside of edk2 is a bad idea.) In particular,
>>>> a collision between MdePkg and MdeModulePkg would be super bad.
>>>>
>>>> Which now seems to turn out consistent with my general review point that
>>>> the [Packages] section, like (almost) all other INF file sections,
>>>> should be sorted lexicographically.
>>>>
>>>> How about replacing
>>>>
>>>> """
>>>> Packages must be listed in the order that may be required for specifying
>>>> include path statements for a compiler. For example, the MdePkg/MdePkg.dec_
>>>> file must be listed before the `MdeModulePkg/MdeModulePkg.dec` file.
>>>> """
>>>>
>>>> with
>>>>
>>>> """
>>>> The order in which packages are listed may be relevant. Said order
>>>> specifies in what order include path statements are generated for a
>>>> compiler. Normally, header file name collisions are not expected between
>>>> packages -- they are forbidden in edk2 proper --, but with a module INF
>>>> consuming both edk2-native and out-of-edk2 packages, header file names
>>>> may collide. For setting specific include path priorities, the packages
>>>> may be listed in matching order in the INF file. Listing a package
>>>> earlier will cause a compiler to consider include paths from that
>>>> package earlier.
>>>> """
>>>
>>> Could I suggest striking:
>>> " -- they are forbidden in edk2 proper --, but with a module INF
>>> consuming both edk2-native and out-of-edk2 packages, header file
>>> names may collide"?
>>
>> I'm sad; that's the part I like the most! ;) That describes the actual
>> use case (I'm a fan of use case details in commit messages too).
>>
>> Anyway, I don't insist...
>>
>>>
>>> This document specifies a file format, not automatically edk2-related.
>>
>> I disagree with this specific statement; the INF spec says "edk2" in the
>> *name*. It's called "edk2 INF specification".
>>
>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Specifications
>>
>> "This page contains the released versions of the EDK II Specifications
>> published using Gitbook."
>>
>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Specifications#inf
>>
>> "This document describes the EDK II build information (INF) file format."
>>
>> The following link doesn't seem to load at the moment:
>>
>> https://edk2-docs.gitbooks.io/edk-ii-inf-specification/content/v/release/1.27/
>>
>> but checking the source in the git repo
>> <https://github.com/tianocore-docs/edk2-InfSpecification>, the actual
>> text seems to say "EDK II Module Information (INF) File Specification".
>>
>> The whole feature is related to out-of-tree INF files (where header file
>> name collisions cannot be easily detected).
> 
> My wording "edk2-related" was imprecise to the level of being
> incorrect, apologies for adding confusion.
> 
> My intended point wast that here is nothing specific about colliding
> with headers in the edk2 repository. This aspect relates to *all*
> different repos used by a specific platform. (And given how much magic
> the EDK2 build system looks to a newcomer anyway...)
> 
>>> I think we're reaching a point where a major documentation overhaul is
>>> necessary. I had already been reflecting on how the coding style
>>> document encompasses more than coding style (at one point it explains
>>> how while() loops are different from do{}while() loops). And we
>>> recently had that conversation around struct assignments which some
>>> maintainers claim are banned, but which is not mentioned in that
>>> document.
>>>
>>> Not trying to resolve that issue *now*, just reflecting on how some
>>> things have been added to these documents historically to deal with a
>>> specific issue, and ended up confusing things as improved development
>>> practices have made the original problem go away.
>>>
>>> So with the edk2 refences removed, I like your new wording.
>>
>> OK -- I won't let "perfect" get in the way of "good" :)
> 
> So, umm, given that the entire actual content of the patch would now
> be written by you - could you submit possibly your own patch, and I'll
> abandon this one?
> 
> I'd be happy with (or frankly, without :) a Suggested-by.

I can add this to my queue, sure. I'll get to it sometime. If it's
urgent, anyone please feel free to post the patch with the updated wording.

Thanks,
Laszlo