cmake: ensure fmt lib is at least 8.1.1 by theanalyst · Pull Request #48705 · ceph/ceph

theanalyst · 2022-11-02T12:29:34Z

Since we depend on newer api features like group_digits ensure that the system version of fmt we use is at least as new as the submodules we bring.

Signed-Off-By: Abhishek Lekshmanan abhishek.l@cern.ch

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

theanalyst · 2022-11-03T07:21:25Z

There is ceph.spec.in also where we set fmt-devel >= 6.3.1, I suppose this can also be changed? But not so sure if many distros package such a newer version.

Matan-B · 2022-11-10T11:13:13Z

There is ceph.spec.in also where we set fmt-devel >= 6.3.1, I suppose this can also be changed? But not so sure if many distros package such a newer version.

Let's update ceph.spec.in as well since we already introduced fmtlib 8.1 features. @ronen-fr

theanalyst · 2022-11-15T13:52:22Z

There is ceph.spec.in also where we set fmt-devel >= 6.3.1, I suppose this can also be changed? But not so sure if many distros package such a newer version.

Let's update ceph.spec.in as well since we already introduced fmtlib 8.1 features. @ronen-fr

So tried this on my centos8 box and here since we don't have system packages of fmtlib > 8.1.1 the install-deps step fails. Don't know what the current distro support matrix is for master (I see fc36 packages newer version), but since we do use fmt as a submodule and anyway can build regardless of system packages don't know whether it is worth failing install-deps. Happy to update ceph.spec.in or keep this pr as it is based on the opinion here.

yehudasa · 2022-12-15T16:45:42Z

I ran into this issue now. This will fix compilation, but my question is whether the original usage of fmt::group_digits() is worth dropping a system package in favor of the submodule version. We should at least figure out which systems that would affect. I think the original change can be easily adapted to avoid the need for the package upgrade.

github-actions · 2022-12-17T20:19:06Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

rzarzynski · 2023-06-27T16:56:00Z

From core PR scrub: @ktdreyer would you mind taking a look? It seems this PR pokes with packages we need to install.

ktdreyer · 2023-07-03T20:03:43Z

I'm sorry, I cannot review this PR now with my other priorities

ktdreyer

I've made time to look at this today, since I've hit this build failure myself.

../src/osd/scrubber/osd_scrub_sched.cc:552:24: error: ‘group_digits’ is not a member of ‘fmt’
            fmt::group_digits(conf()->osd_scrub_max_interval),

On RHEL 8:

fmt is in EPEL 8, fmt-6.2.1-1.el8
In an el8 build: -DWITH_FMT_HEADER_ONLY:BOOL=ON leads to Could not find fmt, will build it

On RHEL 9:

fmt is in EPEL 9, fmt-8.1.1-5.el9
In an el9 build, -DWITH_FMT_HEADER_ONLY:BOOL=ON uses the system package, though CMake's find_package() does not print the version it successfully found (maybe the QUIET option hides that success output?)

I'm good with merging this and backporting to reef.

Separately, I do think the version mis-match in ceph.spec.in is confusing (7.0.0 and 8.1.1 here in CMake vs 6.2.1 in ceph.spec.in). If we bump that number in the RPM packaging, though, then we'll fail to build on el8, since EPEL 8 will no longer be able to satisfy that. My recommendation (separate PR!) is:

Set BuildRequires: fmt-devel >= 8.1.1 behind a conditional so it takes effect only on Fedora and RHEL 9+, not on RHEL 8
Better yet, drop el8 altogether for main

ronen-fr · 2023-07-19T12:04:39Z

@ktdreyer

Better yet, drop el8 altogether for main

I'm wholeheartedly for that.
Note that when discussed in some meetings, the question that was raised was
how dropping centos 8 affects old RHEL support.

ktdreyer · 2023-07-20T13:45:15Z

@ronen-fr Do you mind giving your approval on this PR so we can merge it to main?

ktdreyer · 2023-07-20T16:10:55Z

@theanalyst could you rebase this onto the latest main?

rosinL · 2023-07-24T08:22:49Z

Separately, I do think the version mis-match in ceph.spec.in is confusing (7.0.0 and 8.1.1 here in CMake vs 6.2.1 in ceph.spec.in).

yeah, It is really confusing.

On RHEL 8:
fmt is in EPEL 8, fmt-6.2.1-1.el8
In an el8 build: -DWITH_FMT_HEADER_ONLY:BOOL=ON leads to Could not find fmt, will build it

Why not always building the fmt?
As I remove BuildRequires: fmt-devel >= 6.2.1 from ceph.spec and remove the fmt-devel packages(as many packages depend on fmt, so keep it), ceph building works fine, so I think the BuildRequires of fmt-devel in ceph.spec.in is not really needed.

tchaikov · 2023-07-24T09:07:59Z

Separately, I do think the version mis-match in ceph.spec.in is confusing (7.0.0 and 8.1.1 here in CMake vs 6.2.1 in ceph.spec.in).

yeah, It is really confusing.

On RHEL 8:
fmt is in EPEL 8, fmt-6.2.1-1.el8
In an el8 build: -DWITH_FMT_HEADER_ONLY:BOOL=ON leads to Could not find fmt, will build it

Why not always building the fmt? As I remove BuildRequires: fmt-devel >= 6.2.1 from ceph.spec and remove the fmt-devel packages(as many packages depend on fmt, so keep it), ceph building works fine, so I think the BuildRequires of fmt-devel in ceph.spec.in is not really needed.

i think the idea is to respect the philosophy of how the downstream distributions are structured. in general, package maintainers are encouraged to reuse the the shared libraries and facilities shipped along with the distribution instead of vendering by the packages themselves. have you imagined a distribution full of with packages each of which embeds their own libc? the same applies to other 3rd party libraries like fmtlib. there are more reasons for sure, among other things, like better maintainability like a bug in shared library can be fixed right away, without requiring "make world".

ktdreyer · 2023-07-25T15:30:36Z

Right @tchaikov . Also, pre-built packages lower Ceph's build times.

Since we depend on newer api features like `group_digits` ensure that the system version of fmt we use is at least as new as the submodules we bring. Signed-off-by: Abhishek Lekshmanan <abhishek.l@cern.ch>

theanalyst · 2023-07-26T08:25:18Z

@theanalyst could you rebase this onto the latest main?

done!

ronen-fr · 2023-07-27T14:32:39Z

jenkins test make check

idryomov · 2023-08-14T09:05:11Z

jenkins test make check

idryomov

I went ahead and merged since this has been pending for months. Highlighting Ken's recommendation:

Separately, I do think the version mis-match in ceph.spec.in is confusing (7.0.0 and 8.1.1 here in CMake vs 6.2.1 in ceph.spec.in). If we bump that number in the RPM packaging, though, then we'll fail to build on el8, since EPEL 8 will no longer be able to satisfy that. My recommendation (separate PR!) is:

Set BuildRequires: fmt-devel >= 8.1.1 behind a conditional so it takes effect only on Fedora and RHEL 9+, not on RHEL 8

Better yet, drop el8 altogether for main

idryomov · 2023-08-14T10:00:01Z

Opened a reef backport since fmt::group_digits is used there too.

theanalyst requested review from ronen-fr, rzarzynski and tchaikov November 2, 2022 12:29

github-actions bot added the build/ops label Nov 2, 2022

theanalyst mentioned this pull request Nov 2, 2022

osd/scrub: improving scrub logs #47681

Merged

theanalyst force-pushed the cmake-fmt-versionbump branch from 3885b7a to 0c792ef Compare November 2, 2022 12:37

Matan-B self-requested a review November 10, 2022 11:13

tchaikov approved these changes Nov 10, 2022

View reviewed changes

tchaikov mentioned this pull request Dec 17, 2022

cmake, script: run "make check" with fmtlib v9 #49430

Merged

14 tasks

github-actions bot added the needs-rebase label Dec 17, 2022

tchaikov changed the title ~~cmake: ensure fmt lib is atleast 8.1.1~~ cmake: ensure fmt lib is at least 8.1.1 Dec 19, 2022

ktdreyer approved these changes Jul 19, 2023

View reviewed changes

ronen-fr approved these changes Jul 20, 2023

View reviewed changes

github-actions bot removed the needs-rebase label Jul 20, 2023

tchaikov mentioned this pull request Jul 24, 2023

CMakeLists: correct fmt version to fix compilation error #52255

Closed

14 tasks

cmake: ensure fmt lib is atleast 8.1.1

eead42d

Since we depend on newer api features like `group_digits` ensure that the system version of fmt we use is at least as new as the submodules we bring. Signed-off-by: Abhishek Lekshmanan <abhishek.l@cern.ch>

theanalyst force-pushed the cmake-fmt-versionbump branch from d3556f7 to eead42d Compare July 26, 2023 08:25

ktdreyer approved these changes Jul 27, 2023

View reviewed changes

tchaikov mentioned this pull request Aug 12, 2023

fmt: pickup v9.1.0 #49190

Merged

14 tasks

idryomov merged commit 2f4720d into ceph:main Aug 14, 2023

idryomov reviewed Aug 14, 2023

View reviewed changes

idryomov mentioned this pull request Aug 14, 2023

reef: cmake: ensure fmtlib is at least 8.1.1 #52970

Merged

Conversation

theanalyst commented Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

theanalyst commented Nov 3, 2022

Uh oh!

Matan-B commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theanalyst commented Nov 15, 2022

Uh oh!

yehudasa commented Dec 15, 2022

Uh oh!

github-actions bot commented Dec 17, 2022

Uh oh!

rzarzynski commented Jun 27, 2023

Uh oh!

ktdreyer commented Jul 3, 2023

Uh oh!

ktdreyer left a comment

Choose a reason for hiding this comment

Uh oh!

ronen-fr commented Jul 19, 2023

Uh oh!

ktdreyer commented Jul 20, 2023

Uh oh!

ktdreyer commented Jul 20, 2023

Uh oh!

rosinL commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tchaikov commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktdreyer commented Jul 25, 2023

Uh oh!

theanalyst commented Jul 26, 2023

Uh oh!

ronen-fr commented Jul 27, 2023

Uh oh!

idryomov commented Aug 14, 2023

Uh oh!

idryomov left a comment

Choose a reason for hiding this comment

Uh oh!

idryomov commented Aug 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

theanalyst commented Nov 2, 2022 •

edited

Loading

Matan-B commented Nov 10, 2022 •

edited

Loading

rosinL commented Jul 24, 2023 •

edited

Loading

tchaikov commented Jul 24, 2023 •

edited

Loading