Re: [edk2-devel] MdeModulePkg build fails for AARCH64 on Ubuntu 22.04

["Bob Feng" <bob.c.feng@intel.com>]

+ Huang Long, the bug submitter.

I am OK to revert this patch for this stable tag.

Since this patch break the MdeModulePkg build, also suggest to add this case in edk2 CI.

Thanks,
Bob

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com> 
Sent: Thursday, August 25, 2022 11:44 PM
To: devel@edk2.groups.io; quic_llindhol@quicinc.com; Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; 'Ard Biesheuvel' <ardb@kernel.org>; rebecca@bsdio.com; Chen, Christine <yuwei.chen@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: 'Andrew Fish' <afish@apple.com>; Wang, Jian J <jian.j.wang@intel.com>
Subject: RE: [edk2-devel] MdeModulePkg build fails for AARCH64 on Ubuntu 22.04

Hi Bob,

There was feedback in the Bugzilla on July 5 asking if this is a DSC spec issue or a BaseTools implementation issue.  There was no response to that question.

Why was a spec change not considered for this case to match the BaseTools behavior?

Why were no additional details added to the BZ?

I agree with Leif.  If this change introduced regressions, we should consider a revert and evaluate again after the release.

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif 
> Lindholm
> Sent: Thursday, August 25, 2022 1:49 AM
> To: Feng, Bob C <bob.c.feng@intel.com>; devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>; 'Ard Biesheuvel'
> <ardb@kernel.org>; rebecca@bsdio.com; Chen, Christine 
> <yuwei.chen@intel.com>
> Cc: 'Andrew Fish' <afish@apple.com>; Kinney, Michael D 
> <michael.d.kinney@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] MdeModulePkg build fails for AARCH64 on 
> Ubuntu 22.04
> 
> Hi Bob,
> 
> You are suggesting to knowingly break builds between one stable tag 
> and another on a technicality, for one of the most common (and up to 
> date) build operating systems.
> 
> The purpose of the stable tag is to avoid unplanned breakages. 
> Catching this sort of situation is literally what it is for.
> 
> /
>      Leif
> 
> On 2022-08-24 18:17, Feng, Bob C wrote:
> > Hi Liming,
> >
> > This commit 039bdb4d3e96f9c9264abf135b8a0eef2e2b4860 was to fix the 
> > bug that the basetools implementation dose not follow the
> dsc spec and pass the edk2 CI.
> > The patch was send on June 30, reviewed on July 11 and pushed on 
> > July 17. I think there was enough time to give feedback to
> this change.
> >
> > Why not consider to fix the DSC file?
> >
> > Thanks,
> > Bob
> >
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of 
> > gaoliming via groups.io
> > Sent: Wednesday, August 24, 2022 3:16 PM
> > To: Feng, Bob C <bob.c.feng@intel.com>; devel@edk2.groups.io; 'Ard 
> > Biesheuvel' <ardb@kernel.org>; rebecca@bsdio.com; Chen,
> Christine <yuwei.chen@intel.com>
> > Cc: 'Andrew Fish' <afish@apple.com>; 'Leif Lindholm' 
> > <quic_llindhol@quicinc.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
> > Subject: 回复: [edk2-devel] MdeModulePkg build fails for AARCH64 on 
> > Ubuntu 22.04
> >
> > Bob:
> >   The commit 039bdb4d3e96f9c9264abf135b8a0eef2e2b4860 was merged 
> > into Edk2 on Last month (2022 July 17th). This is very new
> change. It brings the behavior change. But, it doesn't highlight its impact to collect feedback.
> >
> >   I suggest to revert it, because I think we need more time to 
> > discuss this behavior change. Even if we decide to make this
> change, we also need to give other people enough time to update DSC file.
> >
> > Thanks
> > Liming
> >> -----邮件原件-----
> >> 发件人: Feng, Bob C <bob.c.feng@intel.com>
> >> 发送时间: 2022年8月23日 15:40
> >> 收件人: devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>; 
> >> 'Ard Biesheuvel' <ardb@kernel.org>; rebecca@bsdio.com; Chen, 
> >> Christine <yuwei.chen@intel.com>
> >> 抄送: 'Andrew Fish' <afish@apple.com>; 'Leif Lindholm'
> >> <quic_llindhol@quicinc.com>; Kinney, Michael D 
> >> <michael.d.kinney@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
> >> 主题: RE: [edk2-devel] MdeModulePkg build fails for AARCH64 on Ubuntu
> >> 22.04
> >>
> >> Hi Liming,
> >>
> >> Reverting patch may not a good idea, some platforms have done the 
> >> implementation based on the DSC spec, if revert, those platforms 
> >> build will break.
> >> This commit 039bdb4d3e96f9c9264abf135b8a0eef2e2b4860 make the 
> >> Basetools behavior be consistent with DSC spec so I don't think 
> >> it's a regression bug.
> >>
> >> Thanks,
> >> Bob
> >>
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of 
> >> gaoliming via groups.io
> >> Sent: Tuesday, August 23, 2022 9:55 AM
> >> To: 'Ard Biesheuvel' <ardb@kernel.org>; devel@edk2.groups.io; 
> >> rebecca@bsdio.com; Feng, Bob C <bob.c.feng@intel.com>; Chen, 
> >> Christine <yuwei.chen@intel.com>
> >> Cc: 'Andrew Fish' <afish@apple.com>; 'Leif Lindholm'
> >> <quic_llindhol@quicinc.com>; Kinney, Michael D 
> >> <michael.d.kinney@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
> >> Subject: 回复: [edk2-devel] MdeModulePkg build fails for AARCH64 on 
> >> Ubuntu 22.04
> >> Importance: High
> >>
> >> Yuwei:
> >>    The commit 039bdb4d3e96f9c9264abf135b8a0eef2e2b4860 (BaseTools:
> >> Fix DSC LibraryClass precedence rule) introduces this issue. I 
> >> agree with Ard suggestion to revert this change first for this 
> >> stable tag 202208. Then, we can discuss this change in the detail.
> >>
> >> Thanks
> >> Liming
> >>> -----邮件原件-----
> >>> 发件人: Ard Biesheuvel <ardb@kernel.org>
> >>> 发送时间: 2022年8月22日 17:34
> >>> 收件人: devel@edk2.groups.io; rebecca@bsdio.com; Feng, Bob C 
> >>> <bob.c.feng@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>
> >>> 抄送: Andrew Fish <afish@apple.com>; Leif Lindholm 
> >>> <quic_llindhol@quicinc.com>; Michael D Kinney 
> >>> <michael.d.kinney@intel.com>; Jian J Wang <jian.j.wang@intel.com>
> >>> 主题: Re: [edk2-devel] MdeModulePkg build fails for AARCH64 on 
> >>> Ubuntu
> >>> 22.04
> >>>
> >>> On Mon, 22 Aug 2022 at 11:30, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>>>
> >>>> On Mon, 22 Aug 2022 at 11:11, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>>>>
> >>>>> (cc Bob)
> >>>>>
> >>>>> NOTE this affects the stable tag - please see below
> >>>>>
> >>>>>
> >>>>> On Sun, 21 Aug 2022 at 06:34, Rebecca Cran <rebecca@bsdio.com>
> >>> wrote:
> >>>>>>
> >>>>>> I noticed that MdeModulePkg fails to build on my Ubuntu 22.04 
> >>>>>> system (and previously on the Ubuntu 20.04 installation). Is 
> >>>>>> this a
> >> known bug?
> >>>>>>
> >>>>>> It shouldn't be relevant, but I'm using gcc version 11.2.0 
> >>>>>> (Ubuntu 11.2.0-17ubuntu1). But perhaps more relevant, I'm using
> >> Python 3.10.4.
> >>>>>>
> >>>>>> I'm trying to build commit
> >>> e2ac68a23b4954d5c0399913a1df3dd9fd90315d.
> >>>>>>
> >>>>>>
> >>>>>> bcran@photon:~/src/tmp/edk2$ build -p
> >>> MdeModulePkg/MdeModulePkg.dsc -a
> >>>>>> AARCH64 -t GCC5 -b RELEASE
> >>>>>> Build environment:
> >>>>>> Linux-5.15.0-46-generic-x86_64-with-glibc2.35
> >>>>>> Build start time: 22:29:18, Aug.20 2022
> >>>>>>
> >>>>>> WORKSPACE        = /home/bcran/src/tmp/edk2
> >>>>>> EDK_TOOLS_PATH   = /home/bcran/src/tmp/edk2/BaseTools
> >>>>>> CONF_PATH        = /home/bcran/src/tmp/edk2/Conf
> >>>>>> PYTHON_COMMAND   = /usr/bin/python3
> >>>>>>
> >>>>>>
> >>>>>> Processing meta-data
> >>>>>> .Architecture(s)  = AARCH64
> >>>>>> Build target     = RELEASE
> >>>>>> Toolchain        = GCC5
> >>>>>>
> >>>>>> Active Platform          =
> >>>>>> /home/bcran/src/tmp/edk2/MdeModulePkg/MdeModulePkg.dsc
> >>>>>>
> >>>>>>
> >>>>>> build.py...
> >>>>>>
> >>>
> >> /home/bcran/src/tmp/edk2/MdeModulePkg/Library/SmmLockBoxLib/SmmL
> >>> ockBoxPeiLib.inf(42):
> >>>>>> error 3000: PCD
> >>>>>> [gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode] in
> >>>>>>
> >>>
> >> [/home/bcran/src/tmp/edk2/MdeModulePkg/Library/SmmLockBoxLib/SmmL
> >>> ockBoxPeiLib.inf]
> >>>>>> is not found in dependent packages:
> >>>>>>
> >> /home/bcran/src/tmp/edk2/MdePkg/MdePkg.dec
> >>>>>>
> >>> /home/bcran/src/tmp/edk2/MdeModulePkg/MdeModulePkg.dec
> >>>>>>
> >>>>>>
> >>>>>
> >>>>> This looks like a BaseTools regression to me - afaik, these 
> >>>>> packages all used to build for AARCH64 in the past.
> >>>>>
> >>>>> The following LockBoxLib resolutions appear in MdeModulePkg.dsc 
> >>>>> (with line numbers)
> >>>>>
> >>>>> 115-[LibraryClasses.common.PEIM]
> >>>>> 119:
> >>>
> >> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.inf
> >>>>>
> >>>>> 178-[LibraryClasses.ARM, LibraryClasses.AARCH64]
> >>>>> 181:
> >>> LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf
> >>>>>
> >>>>> No other [LlibraryClasses] references to SmmLockBoxPeiLib.inf 
> >>>>> exist in that file, and the [Components] reference does not 
> >>>>> apply to
> >> AARCH64.
> >>>>>
> >>>>> This means the DSC parser ends up using the earlier definition, 
> >>>>> which I think is a bug, or at least a change in behavior. Note 
> >>>>> that doing this silently could potentially break many platforms 
> >>>>> in subtle ways so I think this should be root caused and 
> >>>>> preferably fixed before creating the stable tag.
> >>>>
> >>>> Seems to be deliberate:
> >>>>
> >>>> commit 039bdb4d3e96f9c9264abf135b8a0eef2e2b4860
> >>>> Author: Chen, Christine <Yuwei.Chen@intel.com>
> >>>> Date:   Thu Jun 30 17:04:05 2022 +0800
> >>>>
> >>>>      BaseTools: Fix DSC LibraryClass precedence rule
> >>>>
> >>>> but I don't think the impact on other platforms and drivers has 
> >>>> been taken into account here. Also, the document [0] seems ambiguous to me:
> >>>>
> >>>> """
> >>>> The first globally defined library instance, defined in a DSC 
> >>>> file, that satisfies a module's requirement for a Library Class, 
> >>>> unless specifically overridden by the module in the [Components] 
> >>>> section, will be used.
> >>>> """
> >>>>
> >>>> This says that the first occurrence of a library instance 
> >>>> definition that satisfies a INF dependency will be selected. 
> >>>> Then, there is
> >>>>
> >>>> """
> >>>> The Library Instances will be selected using the following rules 
> >>>> to satisfy a library class for each module listed in the 
> >>>> [Components] section (in order of highest precedence):
> >>>> """
> >>>>
> >>>> which says that not the *first* occurrence, but the occurrence 
> >>>> *with the highest precedence* will be selected.
> >>>>
> >>>> Then, the precedence list has these items:
> >>>>
> >>>> """
> >>>> 2. [LibraryClasses.$(Arch).$(MODULE_TYPE),
> >>>> LibraryClasses.$(Arch).$(MODULE_TYPE)]
> >>>> 3. [LibraryClasses.$(Arch).$(MODULE_TYPE)]
> >>>> """
> >>>>
> >>>> which seem to suggest that a definition that applies to multiple 
> >>>> arch/module_type combinations automatically takes precedence over 
> >>>> one that targets a single arch/module_type combination, which 
> >>>> makes no sense at all.
> >>>>
> >>>> So I strongly suggest we revert the patch, fix the document so it 
> >>>> is clear and ambiguous, and preferably, align it with how it used 
> >>>> to
> >>>
> >>> *un*ambiguous ....
> >>>
> >>> sigh
> >>
> >>
> >>
> >>
> >>
> >>
> >
> >
> >
> >
> >
> >
> >
> >
> 
> 
> 
> 
>