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

[Evan Lloyd <Evan.Lloyd@arm.com>]

Hi Chris.

> -----Original Message-----
> From: Chris Co <Christopher.Co@microsoft.com>
> Sent: 27 June 2018 21:03
> 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
>
> > -----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_GCC49
> xASL\\ARM\\Platform\\SolidRun\\HUMMINGBOARD_EDGE_IMX6Q_2GB\\Ac
> piTables\\AcpiTables\\OUTPUT\\.\\DSDT.i"
> After normpath =
> "e:\rebase\Build\HUMMINGBOARD_EDGE_IMX6Q_2GB\RELEASE_GCC49xAS
> L\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\o
> utput\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.

 [[Evan Lloyd]] This is not my call but, as Leif asked me to stick my oar in, I'll say that I think it would be very acceptable.
Of course, the guy to convince is Liming.

Thank you, again.  (Are you getting the idea I'm pleased with this?)
Evan

>
> > > -----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%7C72f988bf8
> 6
> > > > f141a
> > > > >
> > > >
> > >
> >
> f91ab2d7cd011db47%7C1%7C0%7C636656961074375810&amp;sdata=%2FvF
> v
> > > > 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%2
> > > > > Fl
> > > > > 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=u
> u
> > > > 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.
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.