Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop installing multiple copies of the shared library. #76

Merged
merged 2 commits into from
Dec 5, 2024

Conversation

bodea
Copy link
Collaborator

@bodea bodea commented Dec 5, 2024

This is never linked into programs by ld, so the symlink redirections from .so
and soname are not required. Moreover, the Debian package was ending up with
three copies of the same file (rather than one file and two symlinks).

The only copy will be installed as libnss_cache.so.2, to meet the API for NSS
which expects to load libnss_<service>.so.<x>, where x is 2 for glibc 2.1 and
later (see nsswitch.conf(5)).

bodea added 2 commits December 5, 2024 10:52
This was last updated for 0.17-1 and no longer up to date with the current
Debian packaging.

https://salsa.debian.org/debian/libnss-cache should be the canonical location
for that packaging, and ideally the contents of the debian subirectory will be
the only difference between that repository and this one
(https://github.com/google/libnss-cache).
This is never linked into programs by ld, so the symlink redirections from .so
and soname are not required.  Moreover, the Debian package was ending up with
three copies of the same file (rather than one file and two symlinks).

The only copy will be installed as libnss_cache.so.2, to meet the API for NSS
which expects to load libnss_<service>.so.<x>, where x is 2 for glibc 2.1 and
later (see nsswitch.conf(5)).
Copy link

google-cla bot commented Dec 5, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@bodea bodea merged commit 35d28bf into google:main Dec 5, 2024
5 checks passed
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.80%. Comparing base (08c9bc4) to head (5824715).
Report is 20 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #76       +/-   ##
===========================================
- Coverage   69.97%   58.80%   -11.17%     
===========================================
  Files           2        2               
  Lines         403      403               
  Branches       26       87       +61     
===========================================
- Hits          282      237       -45     
  Misses        100      100               
- Partials       21       66       +45     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kev009
Copy link
Contributor

kev009 commented Dec 5, 2024

This might have been too eager, I am fine with just one library but maybe we need to continue using the SONAME variable?

Warning: pkg(8) will not register it as being provided by the port.
Warning: If another port depend on it, pkg will not be able to know where it comes from.
Warning: It is directly in /usr/local/lib, it is probably used by other ports.```

@bodea
Copy link
Collaborator Author

bodea commented Dec 6, 2024

A SONAME is only required when linking via a libnss_cache.so symlink, so that the linker writes libnss_cache.so.2 into the binary rather than the path name. We're not providing such a symlink, so it is not necessary. If someone were to link to the library for some reason, that would require passing using the full path (rather than using -lnss_cache), in which case the linker records that path correctly.

@kev009
Copy link
Contributor

kev009 commented Dec 6, 2024

A SONAME is only required when linking via a libnss_cache.so symlink, so that the linker writes libnss_cache.so.2 into the binary rather than the path name. We're not providing such a symlink, so it is not necessary. If someone were to link to the library for some reason, that would require passing using the full path (rather than using -lnss_cache), in which case the linker records that path correctly.

Empirically it seems to be used by the pkg system. Can it not be set to the same file name?

@kev009
Copy link
Contributor

kev009 commented Dec 7, 2024

Who even are the people that approved and merged this with no history with the repo. Typical abysmal Google engineering quality...

This is a standard Debian lint as well https://lintian.debian.org/tags/sharedobject-in-library-directory-missing-soname.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants