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

Rework libintern #1312

Merged
merged 3 commits into from
Nov 6, 2024
Merged

Rework libintern #1312

merged 3 commits into from
Nov 6, 2024

Conversation

trws
Copy link
Member

@trws trws commented Nov 4, 2024

intern: fold into fluxion-data and version the so
problem: the libintern so wasn't being versioned, causing problems on
upgrades, see #1308

solution: the real dependency should be on libfluxion-data anyway, which
was closer to how we wanted it. The libintern library is now built
static, linked into libfluxion-data, and provided that way.

Additionally, the libfluxion-data library is installed as
libfluxion-data.so.{version without git suffix} and linked as
libfluxion-data.so.{version major}.{version minor} with no unsuffixed
link installed.

The other two commits here are small fixes that I ran into needing to fix warnings in the cmake build and load properly on my tumbleweed vm.

fixes #1308

@trws trws requested review from grondo and milroy November 4, 2024 20:19
Copy link
Member

@jameshcorbett jameshcorbett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable!

trws added 3 commits November 4, 2024 16:19
problem: the cmake-provided boost finder is deprecated in favor of the
one upstream in boost starting in 1.70

solution: since we support 1.66 and up, attempt to use the upstream
config (use the CONFIG flag on the find_package call), and only if that
fails use the one included with cmake
problem: on my OpenSUSE dev vm, the install directory is forced pretty
hard to use lib64

solution: allow lib64 as a valid path for libraries, packageconfigs and modules
problem: the libintern so wasn't being versioned, causing problems on
upgrades, see flux-framework#1308

solution: the real dependency should be on libfluxion-data anyway, which
was closer to how we wanted it.  The libintern library is now built
static, linked into libfluxion-data, and provided that way.

Additionally, the libfluxion-data library is installed as
`libfluxion-data.so.{version without git suffix}` and linked as
`libfluxion-data.so.{version major}.{version minor}` with no unsuffixed
link installed.
@trws trws force-pushed the rework-libintern branch from ee59d71 to 58458db Compare November 5, 2024 00:19
@trws
Copy link
Member Author

trws commented Nov 5, 2024

@grondo, I think this covers the issue, but want to run it by you before we finalize it.

@garlick
Copy link
Member

garlick commented Nov 5, 2024

This is what I get when I build it:

$ dpkg -c flux-sched_0.39.0-17-g58458db7_amd64.deb|grep libfluxion-data
-rw-r--r-- root/root     43544 2024-11-04 16:36 ./usr/lib/x86_64-linux-gnu/libfluxion-data.so.0.39.0
lrwxrwxrwx root/root         0 2024-11-04 16:36 ./usr/lib/x86_64-linux-gnu/libfluxion-data.so.0.39 -> libfluxion-data.so.0.39.0

That seems OK to me?

@trws
Copy link
Member Author

trws commented Nov 5, 2024

That's what I was shooting for, so we could update on patch if we want to but will multi-version on possibly breaking changes (which right now means minor versions).

Copy link
Member

@milroy milroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks good to me.

@trws trws added the merge-when-passing mergify.io - merge PR automatically once CI passes label Nov 6, 2024
@mergify mergify bot merged commit 1c1f8d6 into flux-framework:master Nov 6, 2024
21 checks passed
Copy link

codecov bot commented Feb 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.2%. Comparing base (5ac0773) to head (58458db).
Report is 36 commits behind head on master.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1312   +/-   ##
======================================
  Coverage    75.2%   75.2%           
======================================
  Files         111     111           
  Lines       15983   15983           
======================================
  Hits        12032   12032           
  Misses       3951    3951           

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-passing mergify.io - merge PR automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fluxion specific libintern.so should have package-specific prefix and version suffix
4 participants