Re: [PATCH v1 1/1] BaseTools/Trim: Canonicalize filepaths to fix comparison

[Chris Co <Christopher.Co@microsoft.com>]

Forgot to add that I'll try out the os.path.normpath/normcase to see if this will work.

> -----Original Message-----
> From: Chris Co
> Sent: Wednesday, June 27, 2018 11:13 AM
> To: 'Evan Lloyd' <Evan.Lloyd@arm.com>; edk2-devel@lists.01.org; Liming
> Gao <liming.gao@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Sami Mujawar
> <Sami.Mujawar@arm.com>
> Subject: RE: [PATCH v1 1/1] BaseTools/Trim: Canonicalize filepaths to fix
> comparison
> 
> Hi Liming, Evan,
> 
> I have attached an example of a generated DSDT.iii file which gets processed
> using Trim.  It will give us a concrete example of what these changes do.
> 
> The first line has its #line directive stripped and the filepath saved as the
> PreProcessedFile.  In GCC49, the filepath was:
> "e:\\rebase\\Build\\HUMMINGBOARD_EDGE_IMX6Q_2GB\\RELEASE_GCC4
> 9xASL\\ARM\\Platform\\SolidRun\\HUMMINGBOARD_EDGE_IMX6Q_2GB\\
> AcpiTables\\AcpiTables\\OUTPUT\\.\\DSDT.i"
> 
> while in GCC5+ it is:
> "e:\\rebase\\build\\hummingboard_edge_imx6q_2gb\\release_gcc5\\arm\
> \platform\\solidrun\\hummingboard_edge_imx6q_2gb\\acpitables\\acpitab
> les\\output\\dsdt.i"
> 
> So there is a difference in both lowercase as well as \\.\\ before dsdt.i.
> 
> As the Trim function goes through each line, it strips the #line directive and
> saves the filepath as InjectedFile, which is then compared against
> PreProcessedFile
> (https://github.com/tianocore/edk2/blob/975478f6bb22668efae311eb3f740
> 6e1f18411c2/BaseTools/Source/Python/Trim/Trim.py#L174)
> If it is not a match, skip to next line.  When using GCC5+, DSDT.iii's #line
> directives after the first keeps the old GCC49 filepath syntax so the string
> comparison always fails.  An example of a line which should match line 602 in
> DSDT.iii.
> 
> So my changes target these two cases directly, but admittedly I'm more of a
> kernel dev than a python dev so if there is a better way to address this, I'm all
> for it.
> 
> > -----Original Message-----
> > From: Evan Lloyd <Evan.Lloyd@arm.com>
> > Sent: Wednesday, June 27, 2018 4:35 AM
> > To: Chris Co <Christopher.Co@microsoft.com>; edk2-devel@lists.01.org
> > Cc: Liming Gao <liming.gao@intel.com>; Leif Lindholm
> > <leif.lindholm@linaro.org>; Sami Mujawar <Sami.Mujawar@arm.com>
> > Subject: RE: [PATCH v1 1/1] BaseTools/Trim: Canonicalize filepaths to
> > fix comparison
> >
> > Hi Chris.
> > Firstly, thank you: this is a useful, pragmatic solution to an major
> annoyance.
> > I personally think it unfortunate that the GCC guys can't be bothered
> > to fix their compiler, but ...
> >
> > > -----Original Message-----
> > > From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of
> > > Chris Co
> > > Sent: 27 June 2018 04:58
> > > To: edk2-devel@lists.01.org
> > > Cc: Liming Gao <liming.gao@intel.com>
> > > Subject: [edk2] [PATCH v1 1/1] BaseTools/Trim: Canonicalize
> > > filepaths to fix comparison
> > >
> > > When using Linaro GCC5+ arm-eabi toolchain on Windows, the generated
> > > DSDT.iii contains a canonicalized ("\.\" removed and lower case)
> > > filepath for the preprocessed DSDT.i file in the first line.
> > > Due to this, when Trim.exe is called to generate DSDT.iiii, future
> > > filepath comparisons against this canonicalized filepath, which
> > > should match, actually fail the comparison which results in an empty
> DSDT.iiii.
> > >
> > > Issue was first reported to Linaro here:
> > >
> >
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.
> > >
> >
> linaro.org%2Fshow_bug.cgi%3Fid%3D2909&amp;data=02%7C01%7CChristop
> > her.C
> > >
> >
> o%40microsoft.com%7C2abbef5e0163411f08e708d5dc220678%7C72f988bf86
> > f141a
> > >
> >
> f91ab2d7cd011db47%7C1%7C0%7C636656961074375810&amp;sdata=%2FvFv
> > maBD0ul
> > > z89CY%2BMaOpwvpTvK9%2FxXjeZeSiXUCYH0%3D&amp;reserved=0
> > > where the recommendation was to address the issue in Trim.exe.
> > >
> > > This patch canonicalizes and lower cases all file paths encountered
> > > during trim execution on preprocessed files.  Since file paths are
> > > standarized, the comparison succeeds for files that should match
> > > regardless of the presence of upper case or "\.\" characters in the file
> path.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Christopher Co <christopher.co@microsoft.com>
> > > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > > Cc: Yonghong Zhu <yonghong.zhu@intel.com>
> > > Cc: Liming Gao <liming.gao@intel.com>
> > > ---
> > >  BaseTools/Source/Python/Trim/Trim.py | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/BaseTools/Source/Python/Trim/Trim.py
> > > b/BaseTools/Source/Python/Trim/Trim.py
> > > index a74075859148..cca4e5c9694a 100644
> > > --- a/BaseTools/Source/Python/Trim/Trim.py
> > > +++ b/BaseTools/Source/Python/Trim/Trim.py
> > > @@ -166,6 +166,8 @@ def TrimPreprocessedFile(Source, Target,
> > > ConvertHex,
> > > TrimLong):
> > >              if len(MatchList) == 2:
> > >                  LineNumber = int(MatchList[0], 0)
> > >                  InjectedFile = MatchList[1]
> > > +                InjectedFile = InjectedFile.replace("\\.\\","")
> >
> > [[Evan Lloyd]] I've not actually tried this yet, but it looks
> > surprizing.  I'd have expected InjectedFile.replace("\\.\\","\\").
> >           Can I ask how it works, please?  Have I missed something?
> > [[Evan Lloyd]] Would it be possible to achieve the same effect with
> > os.path.normpath (see Common/LongFilePathOsPath.py imported via line
> > 17)?
> >
> > > +                InjectedFile = InjectedFile.lower()
> > [[Evan Lloyd]] Similarly, there is "os.path.normcase" - but that may
> > not get the comparison working (because it converts '/' to '\' on Windows).
> >
> > >                  # The first injetcted file must be the preprocessed file itself
> > >                  if PreprocessedFile == "":
> > >                      PreprocessedFile = InjectedFile
> > > --
> > > 2.16.2.gvfs.1.33.gf5370f1
> > >
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
> > > ts
> > > .01.org%2Fmailman%2Flistinfo%2Fedk2-
> > devel&amp;data=02%7C01%7CChristoph
> > >
> >
> er.Co%40microsoft.com%7C2abbef5e0163411f08e708d5dc220678%7C72f988
> > bf86f
> > >
> >
> 141af91ab2d7cd011db47%7C1%7C0%7C636656961074385815&amp;sdata=uu
> > S9M90lN
> > > ewzqS6ScNwbSm8xC9wpdzAFVZhjEu8w2kk%3D&amp;reserved=0
> > IMPORTANT NOTICE: The contents of this email and any attachments are
> > confidential and may also be privileged. If you are not the intended
> > recipient, please notify the sender immediately and do not disclose
> > the contents to any other person, use it for any purpose, or store or
> > copy the information in any medium. Thank you.