#18 Pass all debug information to abigail
Merged 5 years ago by frantisekz. Opened 5 years ago by mkasik.
mkasik/task-abicheck develop  into  master

file modified
+6 -6
@@ -103,7 +103,7 @@ 

          # dot(.) is needed during search to avoid getting result from ppc64le

          # arch while searching fro ppc64

          if (arch + ".") in rpm:

-             if 'debuginfo' in rpm:

+             if ('debuginfo' in rpm) or ('debugsource' in rpm):

                  stable_debuginfo_rpm.append(rpm)

              else:

                  stable_rpms.append(rpm)
@@ -122,7 +122,7 @@ 

          if (arch + ".") in rpm:

          # dot(.) is needed during search to avoid getting result from ppc64le

          # arch while searching fro ppc64

-             if 'debuginfo' in rpm:

+             if ('debuginfo' in rpm) or ('debugsource' in rpm):

                  update_debuginfo_rpm.append(rpm)

              else:

                  update_rpms.append(rpm)
@@ -138,13 +138,13 @@ 

      # dependent packages

      base_command = ['abipkgdiff', '--dso-only']

  

-     if len(stable_debuginfo_rpm) == 1:

+     for rpm in stable_debuginfo_rpm:

          base_command.extend(['--d1',

-                             stable_rpmsdir + '/' + stable_debuginfo_rpm[0]])

+                             stable_rpmsdir + '/' + rpm])

  

-     if len(update_debuginfo_rpm) == 1:

+     for rpm in update_debuginfo_rpm:

          base_command.extend(['--d2',

-                             update_rpmsdir + '/' + update_debuginfo_rpm[0]])

+                             update_rpmsdir + '/' + rpm])

  

      if not ((len(update_rpms_detail) == 0) and (len(stable_rpms_detail) == 0)):

          detail.output.append("On %s architecture\n" % arch)

I've got this log from taskotron yesterday:

https://taskotron.fedoraproject.org/artifacts/all/29343708-e69c-11e8-958c-525400fc9f92/tests.yml/poppler-0.67.0-3.fc29.log

It shows that some symbols which were not referenced by debuginfo were removed and added. I've tried to run it locally and it passed so I had a look into the abicheck code and it seems that you are not passing any debuginfo to abigail at all since debug information is split to several debuginfo packages and one debugsource package now and there is a check that there has to be just 1 debuginfo package. Passing all the packages to the check works for me.
It would be better to pass just the debuginfo for the specific subpackage but I'm not sure how to do that now.

Thanks, Marek!

@sinnykumari can you please check whether the patch is correct and does what it's expected to do?

Thanks Marek for the patch!

      I've got this log from taskotron yesterday:

https://taskotron.fedoraproject.org/artifacts/all/29343708-e69c-11e8-958c-525400fc9f92/tests.yml/poppler-0.67.0-3.fc29.log
It shows that some symbols which were not referenced by debuginfo were removed and added. I've tried to run it locally and it passed so I had a look into the abicheck code and it seems that you are not passing any debuginfo to abigail at all since debug information is split to several debuginfo packages and one debugsource package now and there is a check that there has to be just 1 debuginfo package. Passing all the packages to the check works for me.
It would be better to pass just the debuginfo for the specific subpackage but I'm not sure how to do that now.

It is hard to figure out just by looking at debuginfo filename that which one is the corresponding debuginfo package for a sub-package. Sometimes it is also possible that debuginfo for an rpm requires looking into multiple sub-packages bz#22436
Capability to specify multiple debuginfo option was added a while back to fix this, unfortunately I missed to implement it here.

Thanks a lot for looking at this, patch looks good to me.

Somehow, Merge button is inactive for me.
@kparal feel free to merge it if you can.

@kparal @sinnykumari Merged

(the merge button is disabled for everybody as backend returns 500)

Pull-Request has been closed by frantisekz

5 years ago

Commit e744c48 fixes this pull-request

Pull-Request has been merged by frantisekz

5 years ago

Thank you for merging this.

@kparal @sinnykumari Merged
(the merge button is disabled for everybody as backend returns 500)

Ah ok, thanks!

@frantisekz next time please only merge to develop, let it run for a day in dev to verify it works correctly, and only then merge to master. If this patch was broken, you'd have just broken production.

@kparal Fair point, but this PR was created against master, @sinnykumari owns the repo and approved the merge.

Sorry, I missed it that PR is against master branch. We follow what @kparal said.

I'm sorry for creating the PR against master. I pushed the change to "develop" branch but forgot to check this when asking for the PR.

Metadata