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

["Gao, Liming" <liming.gao@intel.com>]

I understand this issue now. I agree this enhancement to compare the full path.

Thanks
Liming
> -----Original Message-----
> From: Evan Lloyd [mailto:Evan.Lloyd@arm.com]
> Sent: Thursday, June 28, 2018 6:35 PM
> To: Chris Co <Christopher.Co@microsoft.com>; edk2-devel@lists.01.org; Gao, Liming <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 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.