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

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

> -----Original Message-----
> From: Chris Co
> Sent: Wednesday, June 27, 2018 11:23 AM
> To: 'Evan Lloyd' <Evan.Lloyd@arm.com>; 'edk2-devel@lists.01.org' <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
> 
> Forgot to add that I'll try out the os.path.normpath/normcase to see if this
> will work.
> 

Just tried out os.path.normpath and os.path.normcase and together, these calls do achieve the desired result.

Initial filepath string = "e:\\rebase\\Build\\HUMMINGBOARD_EDGE_IMX6Q_2GB\\RELEASE_GCC49xASL\\ARM\\Platform\\SolidRun\\HUMMINGBOARD_EDGE_IMX6Q_2GB\\AcpiTables\\AcpiTables\\OUTPUT\\.\\DSDT.i"
After normpath = "e:\rebase\Build\HUMMINGBOARD_EDGE_IMX6Q_2GB\RELEASE_GCC49xASL\ARM\Platform\SolidRun\HUMMINGBOARD_EDGE_IMX6Q_2GB\AcpiTables\AcpiTables\OUTPUT\DSDT.i"
After normcase = "e:\rebase\build\hummingboard_edge_imx6q_2gb\release_gcc49xasl\arm\platform\solidrun\hummingboard_edge_imx6q_2gb\acpitables\acpitables\output\dsdt.i"

I would prefer to use the os module instead of my direct string manipulation approach so let me know if this change is acceptable and I can submit a v2 using these functions instead.

> > -----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%2Fl
> > > > is
> > > > 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.