[csw-maintainers] New checkpkg test: detect useless libraries links
Yann Rouillard
yann at pleiades.fr.eu.org
Thu Aug 16 22:40:08 CEST 2012
Hi everyone,
I am on the verge on committing the addition of a new check in checkpkg
which is likely to fire some new warnings in a lot of builds, so I am
posting the and requesting some comments before proceeding.
This new check will trigger an error each time a binary is linked against
libraries it doesn't really use.
I worked on this check because I discovered that some programs were linked
against libssl although they didn't really use them, which complicates even
more the upgrade process.
But avoiding useless libraries linking has other advantages:
- it reduces the number of dependancies to install. For exemple, I was
able to remove three dependencies from ssh client, it was dependencies on
libraries required by the server and not by the client.
- it allows to detect some build errors. For exemple, imapproxy was
compiled against libwrap but there was an error in the build system and the
libwrap code was not enabled although imapproxy was linked against libwrap.
- it reduces the number of reverse dependencies impacted when a library is
upgraded and also when it is removed.
This situation seems to happens a lot either because:
1. There is a problem in a configure script which incorrectly add useless
libraries to the linker flags.
For exemple, some programs tries to link with more base libraries that
necessary to be on the safe side, or they tries to link with libraries that
are in fact only needed by their dependencies.
2. There is an error in the pkg-config configuration file of a dependency
which incorrectly requests to link against a larger set of libraries that
needed, sometimes becauses no difference is made between static case and
dynamic case.
That was for exemple a problem with neon, the neon-config tool
incorrectly says that every programs linked against neon should also be
linked against libssl.
3. There are a bunch of binaries provided in a package and the build
system links all of them against the same set of libraries althought some
of them need only to be linked to only a subset of these libraries.
The proper way to fix cases 1 and 2 is a fix in the configure script or
pkg-config script.
The case 3 is harder to fix as build system seldomly make distinction
between the link options of binaries (and this is not a big problem for
Opencsw except if the package is split).
But there is a also direct and easy way to solve all these cases: just add
"-z ignore" to the linker flags and ld will automatically ignore
"-llibrary" option when no symbol of the library is used.
As soon as I commit the patch, a lot of build will trigger this new
checkpkg error so I am thinking about first trying to promote the "-z
ignore" option as a default linker flags for all opencsw builds (following
the same path I proposed for "-B direct"
http://lists.opencsw.org/pipermail/maintainers/2012-August/017113.html ).
I welcome any comment on that.
FYI, the new checkpkg error is documented in the wiki:
http://wiki.opencsw.org/checkpkg-error-tags#toc50
the full patch is attached and you'll find below the patch content without
the tests for comments.
Maciej already reviewed it and gave me useful feedback (thanks to him).
Thanks in advance for your feedbacks,
Yann
Index: lib/python/dependency_checks.py
===================================================================
--- lib/python/dependency_checks.py (revision 18898)
+++ lib/python/dependency_checks.py (working copy)
@@ -39,6 +39,12 @@
PREFERRED_DIRECTORY_PROVIDERS = set([u"CSWcommon"])
+BASE_SOLARIS_LIBRARIES = (
+ "libsocket.so.1", "libnsl.so.1", "libdl.so.1", "librt.so.1",
"libresolv.so.2", "libpthread.so.1",
+ # linked by default with C++, see "Default C++ Libraries" in Solaris
Studio C++ User'sGuide
+ "libCstd.so.1", "libCrun.so.1", "libm.so.1", "libm.so.2",
"libw.so.1", "libcx.so.1", "libc.so.1", "libC.so.3", "libC.so.5"
+)
+
def ProcessSoname(
ldd_emulator,
soname, path_and_pkg_by_basename, binary_info, isalist, binary_path,
logger,
@@ -147,6 +153,20 @@
error_mgr,
pkgname, messenger)
orphan_sonames.extend(orphan_sonames_tmp)
+
+ ldd_info = pkg_data['ldd_info'][binary_info["path"]]
+ for ldd_response in ldd_info:
+ if ldd_response['state'] == 'soname-unused' and
ldd_response['soname'] not in BASE_SOLARIS_LIBRARIES:
+ messenger.Message(
+ "Binary %s links to library %s but doesn't seem to use any of
its symbols. "
+ "It usually happens because superfluous libraries were added to
the linker options, "
+ "either because of the configure script itself or because of the
\"pkg-config --libs\""
+ " output of one the dependency."
+ % ("/" + binary_info["path"], ldd_response['soname']))
+ error_mgr.ReportError(
+ pkgname, "soname-unused",
+ "%s is needed by %s but never used" % (ldd_response['soname'],
"/" + binary_info["path"]))
+
orphan_sonames = set(orphan_sonames)
for soname, binary_path in orphan_sonames:
if soname not in ALLOWED_ORPHAN_SONAMES:
Index: lib/python/package_stats.py
===================================================================
--- lib/python/package_stats.py (revision 18898)
+++ lib/python/package_stats.py (working copy)
@@ -208,6 +208,7 @@
"basic_stats": basic_stats,
"files_metadata": dir_pkg.GetFilesMetadata(),
"mtime": self.GetMtime(),
+ "ldd_info": dir_pkg.GetLddMinusRlines(),
}
self.SaveStats(pkg_stats)
logging.debug("Statistics of %s have been collected.",
repr(dir_pkg.pkgname))
Index: lib/python/inspective_package.py
===================================================================
--- lib/python/inspective_package.py (revision 18898)
+++ lib/python/inspective_package.py (working copy)
@@ -231,15 +231,14 @@
def GetLddMinusRlines(self):
"""Returns ldd -r output."""
- dir_pkg = self.GetInspectivePkg()
- binaries = dir_pkg.ListBinaries()
+ binaries = self.ListBinaries()
ldd_output = {}
for binary in binaries:
- binary_abspath = os.path.join(dir_pkg.directory, "root", binary)
+ binary_abspath = os.path.join(self.directory, "root", binary)
# this could be potentially moved into the DirectoryFormatPackage
class.
# ldd needs the binary to be executable
os.chmod(binary_abspath, 0755)
- args = ["ldd", "-r", binary_abspath]
+ args = ["ldd", "-Ur", binary_abspath]
ldd_proc = subprocess.Popen(
args,
stdout=subprocess.PIPE,
@@ -250,7 +249,9 @@
logging.error("%s returned an error: %s", args, stderr)
ldd_info = []
for line in stdout.splitlines():
- ldd_info.append(self._ParseLddDashRline(line))
+ result = self._ParseLddDashRline(line, binary_abspath)
+ if result:
+ ldd_info.append(result)
ldd_output[binary] = ldd_info
return ldd_output
@@ -263,7 +264,7 @@
sym = { 'address': fields[0], 'type': fields[1], 'name': fields[2] }
return sym
- def _ParseLddDashRline(self, line):
+ def _ParseLddDashRline(self, line, binary=None):
found_re = r"^\t(?P<soname>\S+)\s+=>\s+(?P<path_found>\S+)"
symbol_not_found_re = (r"^\tsymbol not found:\s(?P<symbol>\S+)\s+"
r"\((?P<path_not_found>\S+)\)")
@@ -280,12 +281,17 @@
r'file (?P<sizediff_file2>\S+) size=(?P<size2>0x\w+)\)$')
sizes_one_used = (r'^\t\t(?P<sizediffused_file>\S+) size used; '
r'possible insufficient data copied$')
- common_re = (r"(%s|%s|%s|%s|%s|%s|%s|%s)"
- % (found_re, symbol_not_found_re, only_so, version_so,
- stv_protected, sizes_differ, sizes_info,
sizes_one_used))
+ unreferenced_object = (r'^\s*unreferenced object=(?P<object>.*);
unused dependency of (?P<binary>.*)$')
+ unused_object = (r'^\s*unused object=.*$')
+ unused_search_path = (r'^\s*unused search path=.* \(RUNPATH/RPATH
from file .*\)$')
+ blank_line = (r'^\s*$')
+ common_re = (r"(%s|%s|%s|%s|%s|%s|%s|%s|%s|%s|%s|%s)"
+ % (found_re, symbol_not_found_re, only_so, version_so,
stv_protected, sizes_differ, sizes_info,
+ sizes_one_used, unreferenced_object, unused_object,
unused_search_path, blank_line))
m = re.match(common_re, line)
- response = {}
+ response = None
if m:
+ response = {}
d = m.groupdict()
if "soname" in d and d["soname"]:
# it was found
@@ -298,6 +304,11 @@
response["soname"] = None
response["path"] = d["path_not_found"]
response["symbol"] = d["symbol"]
+ elif "binary" in d and d["binary"] and binary == d["binary"]:
+ response["state"] = "soname-unused"
+ response["soname"] = os.path.basename(d["object"])
+ response["path"] = None
+ response["symbol"] = None
elif d["path_only"]:
response["state"] = "OK"
response["soname"] = None
@@ -328,12 +339,11 @@
response["soname"] = None
response["path"] = "%s" % (d["sizediffused_file"])
response["symbol"] = None
- else:
- raise StdoutSyntaxError("Could not parse %s with %s"
- % (repr(line), common_re))
+
else:
- raise StdoutSyntaxError("Could not parse %s with %s"
- % (repr(line), common_re))
+ raise package.StdoutSyntaxError("Could not parse %s with %s"
+ % (repr(line), common_re))
+
return response
def GetDependencies(self):
Index: lib/python/package.py
===================================================================
--- lib/python/package.py (revision 18898)
+++ lib/python/package.py (working copy)
@@ -42,6 +42,8 @@
class PackageError(Error):
pass
+class StdoutSyntaxError(Error):
+ pass
class CswSrv4File(shell.ShellMixin, object):
"""Represents a package in the srv4 format (pkg)."""
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.opencsw.org/pipermail/maintainers/attachments/20120816/527aa9a0/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: enable_ldd_r.patch
Type: application/octet-stream
Size: 31539 bytes
Desc: not available
URL: <http://lists.opencsw.org/pipermail/maintainers/attachments/20120816/527aa9a0/attachment-0001.obj>
More information about the maintainers
mailing list