[Unstable,1/6] UBUNTU: [Debian]: Remove hardcoded $(pkgdir) in debug symbols handling
diff mbox series

Message ID 20190828122604.6829-2-mfo@canonical.com
State New
Headers show
Series
  • Add support for ZFS debug symbols
Related show

Commit Message

Mauricio Faria de Oliveira Aug. 28, 2019, 12:25 p.m. UTC
BugLink: https://bugs.launchpad.net/bugs/1840704

The 'find .ko | sed | while read module' loop has the $(pkgdir) path
hardcoded in a couple places to reconstruct the path 'sed' destroyed.

Remove that 'sed' expression to destroy the first components of the
absolute pathname and get its '/lib/modules/'-based path with shell.

This is needed for the next patch.

Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
---
 debian/rules.d/2-binary-arch.mk | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Seth Forshee Sept. 5, 2019, 4:06 p.m. UTC | #1
On Wed, Aug 28, 2019 at 09:25:59AM -0300, Mauricio Faria de Oliveira wrote:
> BugLink: https://bugs.launchpad.net/bugs/1840704
> 
> The 'find .ko | sed | while read module' loop has the $(pkgdir) path
> hardcoded in a couple places to reconstruct the path 'sed' destroyed.
> 
> Remove that 'sed' expression to destroy the first components of the
> absolute pathname and get its '/lib/modules/'-based path with shell.
> 
> This is needed for the next patch.
> 
> Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
> ---
>  debian/rules.d/2-binary-arch.mk | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/debian/rules.d/2-binary-arch.mk b/debian/rules.d/2-binary-arch.mk
> index 083b64772439..730373e93e53 100644
> --- a/debian/rules.d/2-binary-arch.mk
> +++ b/debian/rules.d/2-binary-arch.mk
> @@ -282,16 +282,17 @@ ifneq ($(skipdbg),true)
>  		INSTALL_MOD_PATH=$(dbgpkgdir)/usr/lib/debug
>  	# Add .gnu_debuglink sections to each stripped .ko
>  	# pointing to unstripped verson
> -	find $(pkgdir) -name '*.ko' | sed 's|$(pkgdir)||'| while read module ; do \
> +	find $(pkgdir) -name '*.ko' | while read path_module ; do \
> +		module="/lib/modules/$${path_module#*/lib/modules/}"; \

Why not do this instead?

  module=$${path_module#$(pkgdir)}

I don't think it makes a difference in practice, I just find it easier
to parse. I had to stare at what you wrote for a bit to convince myself
that the result was the same as what sed was doing previously, whereas
the above is more obviously equivalent.

Thanks,
Seth
Mauricio Faria de Oliveira Sept. 5, 2019, 6:02 p.m. UTC | #2
On Thu, Sep 5, 2019 at 1:06 PM Seth Forshee <seth.forshee@canonical.com> wrote:
>
> On Wed, Aug 28, 2019 at 09:25:59AM -0300, Mauricio Faria de Oliveira wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1840704
> >
> > The 'find .ko | sed | while read module' loop has the $(pkgdir) path
> > hardcoded in a couple places to reconstruct the path 'sed' destroyed.
> >
> > Remove that 'sed' expression to destroy the first components of the
> > absolute pathname and get its '/lib/modules/'-based path with shell.
> >
> > This is needed for the next patch.
> >
> > Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
> > ---
> >  debian/rules.d/2-binary-arch.mk | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/debian/rules.d/2-binary-arch.mk b/debian/rules.d/2-binary-arch.mk
> > index 083b64772439..730373e93e53 100644
> > --- a/debian/rules.d/2-binary-arch.mk
> > +++ b/debian/rules.d/2-binary-arch.mk
> > @@ -282,16 +282,17 @@ ifneq ($(skipdbg),true)
> >               INSTALL_MOD_PATH=$(dbgpkgdir)/usr/lib/debug
> >       # Add .gnu_debuglink sections to each stripped .ko
> >       # pointing to unstripped verson
> > -     find $(pkgdir) -name '*.ko' | sed 's|$(pkgdir)||'| while read module ; do \
> > +     find $(pkgdir) -name '*.ko' | while read path_module ; do \
> > +             module="/lib/modules/$${path_module#*/lib/modules/}"; \
>
> Why not do this instead?
>
>   module=$${path_module#$(pkgdir)}
>

This would again depend on $(pkgdir) inside the loop,
and the intention is to generalize that for the next patch.

The next patch needs this dependency not to exist,
so that $(dbgpkgdir) can be iterated over too in the loop.

Hope this clarifies things.  Thanks for reviewing!

> I don't think it makes a difference in practice, I just find it easier
> to parse. I had to stare at what you wrote for a bit to convince myself
> that the result was the same as what sed was doing previously, whereas
> the above is more obviously equivalent.
>
> Thanks,
> Seth
Mauricio Faria de Oliveira Sept. 5, 2019, 6:06 p.m. UTC | #3
On Thu, Sep 5, 2019 at 3:02 PM Mauricio Faria de Oliveira
<mfo@canonical.com> wrote:
>
> On Thu, Sep 5, 2019 at 1:06 PM Seth Forshee <seth.forshee@canonical.com> wrote:
> >
> > On Wed, Aug 28, 2019 at 09:25:59AM -0300, Mauricio Faria de Oliveira wrote:
> > > BugLink: https://bugs.launchpad.net/bugs/1840704
> > >
> > > The 'find .ko | sed | while read module' loop has the $(pkgdir) path
> > > hardcoded in a couple places to reconstruct the path 'sed' destroyed.
> > >
> > > Remove that 'sed' expression to destroy the first components of the
> > > absolute pathname and get its '/lib/modules/'-based path with shell.
> > >
> > > This is needed for the next patch.
> > >
> > > Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
> > > ---
> > >  debian/rules.d/2-binary-arch.mk | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/debian/rules.d/2-binary-arch.mk b/debian/rules.d/2-binary-arch.mk
> > > index 083b64772439..730373e93e53 100644
> > > --- a/debian/rules.d/2-binary-arch.mk
> > > +++ b/debian/rules.d/2-binary-arch.mk
> > > @@ -282,16 +282,17 @@ ifneq ($(skipdbg),true)
> > >               INSTALL_MOD_PATH=$(dbgpkgdir)/usr/lib/debug
> > >       # Add .gnu_debuglink sections to each stripped .ko
> > >       # pointing to unstripped verson
> > > -     find $(pkgdir) -name '*.ko' | sed 's|$(pkgdir)||'| while read module ; do \
> > > +     find $(pkgdir) -name '*.ko' | while read path_module ; do \
> > > +             module="/lib/modules/$${path_module#*/lib/modules/}"; \
> >
> > Why not do this instead?
> >
> >   module=$${path_module#$(pkgdir)}
> >
>
> This would again depend on $(pkgdir) inside the loop,
> and the intention is to generalize that for the next patch.
>
> The next patch needs this dependency not to exist,
> so that $(dbgpkgdir) can be iterated over too in the loop.

^ oops, s/dbgpkgdir/pkgdir_ex/ (for linux-modules-extras).
>
> Hope this clarifies things.  Thanks for reviewing!
>
> > I don't think it makes a difference in practice, I just find it easier
> > to parse. I had to stare at what you wrote for a bit to convince myself
> > that the result was the same as what sed was doing previously, whereas
> > the above is more obviously equivalent.
> >
> > Thanks,
> > Seth
>
>
>
> --
> Mauricio Faria de Oliveira
Seth Forshee Sept. 5, 2019, 6:45 p.m. UTC | #4
On Thu, Sep 05, 2019 at 03:06:38PM -0300, Mauricio Faria de Oliveira wrote:
> On Thu, Sep 5, 2019 at 3:02 PM Mauricio Faria de Oliveira
> <mfo@canonical.com> wrote:
> >
> > On Thu, Sep 5, 2019 at 1:06 PM Seth Forshee <seth.forshee@canonical.com> wrote:
> > >
> > > On Wed, Aug 28, 2019 at 09:25:59AM -0300, Mauricio Faria de Oliveira wrote:
> > > > BugLink: https://bugs.launchpad.net/bugs/1840704
> > > >
> > > > The 'find .ko | sed | while read module' loop has the $(pkgdir) path
> > > > hardcoded in a couple places to reconstruct the path 'sed' destroyed.
> > > >
> > > > Remove that 'sed' expression to destroy the first components of the
> > > > absolute pathname and get its '/lib/modules/'-based path with shell.
> > > >
> > > > This is needed for the next patch.
> > > >
> > > > Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
> > > > ---
> > > >  debian/rules.d/2-binary-arch.mk | 7 ++++---
> > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/debian/rules.d/2-binary-arch.mk b/debian/rules.d/2-binary-arch.mk
> > > > index 083b64772439..730373e93e53 100644
> > > > --- a/debian/rules.d/2-binary-arch.mk
> > > > +++ b/debian/rules.d/2-binary-arch.mk
> > > > @@ -282,16 +282,17 @@ ifneq ($(skipdbg),true)
> > > >               INSTALL_MOD_PATH=$(dbgpkgdir)/usr/lib/debug
> > > >       # Add .gnu_debuglink sections to each stripped .ko
> > > >       # pointing to unstripped verson
> > > > -     find $(pkgdir) -name '*.ko' | sed 's|$(pkgdir)||'| while read module ; do \
> > > > +     find $(pkgdir) -name '*.ko' | while read path_module ; do \
> > > > +             module="/lib/modules/$${path_module#*/lib/modules/}"; \
> > >
> > > Why not do this instead?
> > >
> > >   module=$${path_module#$(pkgdir)}
> > >
> >
> > This would again depend on $(pkgdir) inside the loop,
> > and the intention is to generalize that for the next patch.
> >
> > The next patch needs this dependency not to exist,
> > so that $(dbgpkgdir) can be iterated over too in the loop.
> 
> ^ oops, s/dbgpkgdir/pkgdir_ex/ (for linux-modules-extras).
> >
> > Hope this clarifies things.  Thanks for reviewing!

Yes it does, thanks!

> >
> > > I don't think it makes a difference in practice, I just find it easier
> > > to parse. I had to stare at what you wrote for a bit to convince myself
> > > that the result was the same as what sed was doing previously, whereas
> > > the above is more obviously equivalent.
> > >
> > > Thanks,
> > > Seth
> >
> >
> >
> > --
> > Mauricio Faria de Oliveira
> 
> 
> 
> -- 
> Mauricio Faria de Oliveira

Patch
diff mbox series

diff --git a/debian/rules.d/2-binary-arch.mk b/debian/rules.d/2-binary-arch.mk
index 083b64772439..730373e93e53 100644
--- a/debian/rules.d/2-binary-arch.mk
+++ b/debian/rules.d/2-binary-arch.mk
@@ -282,16 +282,17 @@  ifneq ($(skipdbg),true)
 		INSTALL_MOD_PATH=$(dbgpkgdir)/usr/lib/debug
 	# Add .gnu_debuglink sections to each stripped .ko
 	# pointing to unstripped verson
-	find $(pkgdir) -name '*.ko' | sed 's|$(pkgdir)||'| while read module ; do \
+	find $(pkgdir) -name '*.ko' | while read path_module ; do \
+		module="/lib/modules/$${path_module#*/lib/modules/}"; \
 		if [[ -f "$(dbgpkgdir)/usr/lib/debug/$$module" ]] ; then \
 			$(CROSS_COMPILE)objcopy \
 				--add-gnu-debuglink=$(dbgpkgdir)/usr/lib/debug/$$module \
-				$(pkgdir)/$$module; \
+				$$path_module; \
 			if grep -q CONFIG_MODULE_SIG=y $(builddir)/build-$*/.config; then \
 				$(builddir)/build-$*/scripts/sign-file $(MODHASHALGO) \
 					$(MODSECKEY) \
 					$(MODPUBKEY) \
-					$(pkgdir)/$$module; \
+					$$path_module; \
 			fi; \
 		fi; \
 	done