Re: [OE-core] [PATCH] glib-2.0: call os.path.normpath on THISDIR

From: "Richard Purdie" <richard.purdie@linuxfoundation.org>
To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>,
	Martin Jansa <martin.jansa@gmail.com>
Cc: "openembedded-core@lists.openembedded.org"
	<openembedded-core@lists.openembedded.org>,
	"anibal.limon@linaro.org" <anibal.limon@linaro.org>,
	"geoffhp@gmail.com" <geoffhp@gmail.com>
Subject: Re: [OE-core] [PATCH] glib-2.0: call os.path.normpath on THISDIR
Date: Fri, 05 Mar 2021 17:54:25 +0000	[thread overview]
Message-ID: <b9ceb42871b6bfa49caaf1bb86ae6627154688d4.camel@linuxfoundation.org> (raw)
In-Reply-To: <38592c5c6824461687563ed1a52913eb@XBOX03.axis.com>

On Fri, 2021-03-05 at 15:52 +0000, Peter Kjellerstedt wrote:
> > -----Original Message-----
> > From: Martin Jansa <martin.jansa@gmail.com>
> > Sent: den 5 mars 2021 12:51
> > To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> > Cc: openembedded-core@lists.openembedded.org; anibal.limon@linaro.org;
> > > 
> > > Actually, thinking a bit more on this, I believe that with my patch
> > > applied, this patch should _not_ be applied. That is because it makes
> > > the paths relative to thisdir, but then uses ${THISDIR} when using
> > > them. Thus if thisdir is normalized, it may no longer match ${THISDIR}
> > > and the relative paths will not work as intended.
> > 
> > I'm sorry I've tested and wrote that part of commit message when I had
> > if os.path.exists(filename):
> > at the beginning of this whole section, which I've then re-wrote to just
> > store only existing filenames in files as a list instead of a map (to
> > simplify the rest of this function as well), but then I was wondering
> > why Ross used the map in the initial implementation here:
> > https://git.openembedded.org/openembedded-
> > core/commit/?id=5acd9cbc9d5c6355010775250fb25f043441c5cd
> > and assumed it was needed to get all possible filenames in
> > do_configure[file-checksums] in case they are created later.
> > 
> > So I've removed all my cleanups and returned to smallest change to just
> > replace COREBASE with THISDIR.
> > 
> > What about checking the existence before showing the error, that will
> > show an error only when matching file exists while keeping all
> > not-skipped paths in do_configure[file-checksums] even when they don't
> > exist yet.
> > 
> > Will send a patch for this shortly.
> 
> Any objections to my patch, where there is no need for the bb.error() 
> in the first place?

Your patch encodes the paths of layers relative to each other into the 
sstate checksums. I'm not sure we want to encourage that and this code 
will end up copy and pasted around without people realising what they're 
doing :/.

Cheers,

Richard