[b,c,d,v2] UBUNTU: [Packaging] buildinfo -- include origin package mark to modules files
diff mbox series

Message ID 20181205171644.29784-1-marcelo.cerri@canonical.com
State New
Headers show
Series
  • [b,c,d,v2] UBUNTU: [Packaging] buildinfo -- include origin package mark to modules files
Related show

Commit Message

Marcelo Henrique Cerri Dec. 5, 2018, 5:16 p.m. UTC
BugLink: http://bugs.launchpad.net/bugs/1806380

Include a flag at the end of each line of the
"debian.$branch/abi/*/$arch/$flavour.modules" file indicating the
package that each module is currently shipped in.

This will cause the build to fail when a module is silently moved from
or to the linux-modules-extra package.

Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
---

v2: Just fixed the initialization of "mark" in debian/scripts/misc/getabis. Updated
    the same initialization in debian/rules.d/2-binary-arch.mk for consistency.

---
 debian/rules.d/2-binary-arch.mk | 12 ++++++++++--
 debian/scripts/misc/getabis     | 17 +++++++++++++----
 2 files changed, 23 insertions(+), 6 deletions(-)

Comments

Seth Forshee Dec. 10, 2018, 8:50 p.m. UTC | #1
On Wed, Dec 05, 2018 at 03:16:44PM -0200, Marcelo Henrique Cerri wrote:
> BugLink: http://bugs.launchpad.net/bugs/1806380
> 
> Include a flag at the end of each line of the
> "debian.$branch/abi/*/$arch/$flavour.modules" file indicating the
> package that each module is currently shipped in.
> 
> This will cause the build to fail when a module is silently moved from
> or to the linux-modules-extra package.
> 
> Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>

Doesn't module-check also need to be updated to avoid breaking use of
modules.ignore? Or is the expectation that we will start appending the
"extra" as needed when adding modules to that file?

I'm also curious whether you considered splitting them out into multiple
abi files, something like generic.modules and generic.extras.modules,
and if so why you settled on using a flag instead.

Should we extract the flag name from the package name rather than
hardcoding it like you've done here. This may be overthinking it, since
I don't know whether we would need to track abi for any additional
module packages nor what those packages would be named.

Seth
Stefan Bader Jan. 7, 2019, 5:54 p.m. UTC | #2
On 10.12.18 21:50, Seth Forshee wrote:
> On Wed, Dec 05, 2018 at 03:16:44PM -0200, Marcelo Henrique Cerri wrote:
>> BugLink: http://bugs.launchpad.net/bugs/1806380
>>
>> Include a flag at the end of each line of the
>> "debian.$branch/abi/*/$arch/$flavour.modules" file indicating the
>> package that each module is currently shipped in.
>>
>> This will cause the build to fail when a module is silently moved from
>> or to the linux-modules-extra package.
>>
>> Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
> 
> Doesn't module-check also need to be updated to avoid breaking use of
> modules.ignore? Or is the expectation that we will start appending the
> "extra" as needed when adding modules to that file?
> 
> I'm also curious whether you considered splitting them out into multiple
> abi files, something like generic.modules and generic.extras.modules,
> and if so why you settled on using a flag instead.
> 
> Should we extract the flag name from the package name rather than
> hardcoding it like you've done here. This may be overthinking it, since
> I don't know whether we would need to track abi for any additional
> module packages nor what those packages would be named.
> 
> Seth
> 
> 
I'd like to see some reply to Seth's concerns/questions.

-Stefan
Marcelo Henrique Cerri Feb. 4, 2019, 12:13 p.m. UTC | #3
Sorry, I totally missed that one. Answers bellow.

On Mon, Dec 10, 2018 at 02:50:19PM -0600, Seth Forshee wrote:
> On Wed, Dec 05, 2018 at 03:16:44PM -0200, Marcelo Henrique Cerri wrote:
> > BugLink: http://bugs.launchpad.net/bugs/1806380
> > 
> > Include a flag at the end of each line of the
> > "debian.$branch/abi/*/$arch/$flavour.modules" file indicating the
> > package that each module is currently shipped in.
> > 
> > This will cause the build to fail when a module is silently moved from
> > or to the linux-modules-extra package.
> > 
> > Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
> 
> Doesn't module-check also need to be updated to avoid breaking use of
> modules.ignore? Or is the expectation that we will start appending the
> "extra" as needed when adding modules to that file?

I believe so. Good catch. The kernels that I have tested do not use
ignore.modules. I can update the patch with the fix to module-check.

> 
> I'm also curious whether you considered splitting them out into multiple
> abi files, something like generic.modules and generic.extras.modules,
> and if so why you settled on using a flag instead.
>

Yes. My only concern about that approach is that we would multiple the
number of files. Not a big problem. Do you see any benefits of having
a separated file for that?

> Should we extract the flag name from the package name rather than
> hardcoding it like you've done here. This may be overthinking it, since
> I don't know whether we would need to track abi for any additional
> module packages nor what those packages would be named.
>

That can be done. However we already hard code the names of the
packages we use to extract that information ("$(pkgdir_bin) $(pkgdir)
$(pkgdir_ex)").

> Seth
>


--
Regards,
Marcelo
Seth Forshee Feb. 4, 2019, 3:34 p.m. UTC | #4
On Mon, Feb 04, 2019 at 10:13:50AM -0200, Marcelo Henrique Cerri wrote:
> Sorry, I totally missed that one. Answers bellow.
> 
> On Mon, Dec 10, 2018 at 02:50:19PM -0600, Seth Forshee wrote:
> > On Wed, Dec 05, 2018 at 03:16:44PM -0200, Marcelo Henrique Cerri wrote:
> > > BugLink: http://bugs.launchpad.net/bugs/1806380
> > > 
> > > Include a flag at the end of each line of the
> > > "debian.$branch/abi/*/$arch/$flavour.modules" file indicating the
> > > package that each module is currently shipped in.
> > > 
> > > This will cause the build to fail when a module is silently moved from
> > > or to the linux-modules-extra package.
> > > 
> > > Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
> > 
> > Doesn't module-check also need to be updated to avoid breaking use of
> > modules.ignore? Or is the expectation that we will start appending the
> > "extra" as needed when adding modules to that file?
> 
> I believe so. Good catch. The kernels that I have tested do not use
> ignore.modules. I can update the patch with the fix to module-check.

Thanks.

> > 
> > I'm also curious whether you considered splitting them out into multiple
> > abi files, something like generic.modules and generic.extras.modules,
> > and if so why you settled on using a flag instead.
> >
> 
> Yes. My only concern about that approach is that we would multiple the
> number of files. Not a big problem. Do you see any benefits of having
> a separated file for that?

I don't think we should worry too much about whether or not it adds
files to the abi. I was more curious than anything though, I don't have
a strong opinion.

> 
> > Should we extract the flag name from the package name rather than
> > hardcoding it like you've done here. This may be overthinking it, since
> > I don't know whether we would need to track abi for any additional
> > module packages nor what those packages would be named.
> >
> 
> That can be done. However we already hard code the names of the
> packages we use to extract that information ("$(pkgdir_bin) $(pkgdir)
> $(pkgdir_ex)").

Ok, whatever makes the most sense.

Thanks,
Seth
Marcelo Henrique Cerri Feb. 6, 2019, 11:48 a.m. UTC | #5
I will provide a v3.

Patch
diff mbox series

diff --git a/debian/rules.d/2-binary-arch.mk b/debian/rules.d/2-binary-arch.mk
index 20f744c012a9..974dbf04d2f7 100644
--- a/debian/rules.d/2-binary-arch.mk
+++ b/debian/rules.d/2-binary-arch.mk
@@ -407,8 +407,16 @@  endif
 		$(builddir)/build-$*/Module.symvers | sort > $(abidir)/$*
 
 	# Build the final ABI modules information.
-	find $(pkgdir_bin) $(pkgdir) $(pkgdir_ex) -name \*.ko | \
-		sed -e 's/.*\/\([^\/]*\)\.ko/\1/' | sort > $(abidir)/$*.modules
+	> "$(abidir)/$*.modules"; \
+	for dir in "$(pkgdir_bin)" "$(pkgdir)" "$(pkgdir_ex)"; do \
+		case "$$dir" in \
+			*extra*) mark=' extra';; \
+			*) mark='';; \
+		esac; \
+		find "$$dir" -name \*.ko | \
+			sed -e 's/.*\/\([^\/]*\)\.ko/\1'"$$mark"'/' >> "$(abidir)/$*.modules"; \
+	done; \
+	sort -o "$(abidir)/$*.modules" "$(abidir)/$*.modules"
 
 	# Build the final ABI firmware information.
 	find $(pkgdir_bin) $(pkgdir) $(pkgdir_ex) -name \*.ko | \
diff --git a/debian/scripts/misc/getabis b/debian/scripts/misc/getabis
index 42690b0311e2..5d9daa023813 100755
--- a/debian/scripts/misc/getabis
+++ b/debian/scripts/misc/getabis
@@ -65,8 +65,20 @@  getall() {
 			echo -n "extracting$prefixes..."
 			for filename in $filenames
 			do
-				dpkg-deb --extract $filename tmp
+				dpkg-deb --extract "$filename" tmp
+				# Extract the modules list, so we can mark each line
+				# with its origin.
+				case "$filename" in
+					*extra*) mark=' extra';;
+					*) mark='';;
+				esac
+				files=$(dpkg-deb --vextract "$filename" tmp)
+				[ "$?" -ne 0 ] && continue
+				echo "$files" | sed -n -e '/.*\/\([^\/]*\)\.ko/s//\1'"$mark"'/p' >> \
+						    "$abidir/$arch/$sub.modules"
+
 			done
+			sort -o "$abidir/$arch/$sub.modules" "$abidir/$arch/$sub.modules"
 			# FORM 1: linux-image et al extracted here.
 			if [ -d tmp/boot ]; then
 				echo -n "images..."
@@ -83,9 +95,6 @@  getall() {
 				else
 					echo -n "NO RETPOLINE FILE..."
 				fi
-				(cd tmp; find lib/modules/$verabi-$sub/kernel -name '*.ko') | \
-					sed -e 's/.*\/\([^\/]*\)\.ko/\1/' | sort > \
-					$abidir/$arch/$sub.modules
 				(
 					cd tmp;
 					# Prevent exposing some errors when called by python scripts. SIGPIPE seems to get