Skip to content

cephfs: include cleanup#62948

Merged
vshankar merged 11 commits intoceph:mainfrom
MaxKellermann:cephfs_includes
Aug 4, 2025
Merged

cephfs: include cleanup#62948
vshankar merged 11 commits intoceph:mainfrom
MaxKellermann:cephfs_includes

Conversation

@MaxKellermann
Copy link
Member

@MaxKellermann MaxKellermann commented Apr 24, 2025

Another PR split from #60490

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)
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
  • jenkins test rook e2e

@MaxKellermann MaxKellermann requested a review from a team as a code owner April 24, 2025 12:33
@MaxKellermann MaxKellermann force-pushed the cephfs_includes branch 2 times, most recently from 0622ebb to d092c7b Compare April 24, 2025 12:37
@MaxKellermann
Copy link
Member Author

MaxKellermann commented Apr 24, 2025

Linking of some unit tests fails because linker rules for internal rules are missing, which becomes visible with un-inlined symbols. I'll fix that (tomorrow).

@MaxKellermann MaxKellermann requested a review from a team as a code owner April 25, 2025 09:16
@MaxKellermann
Copy link
Member Author

MaxKellermann commented Apr 25, 2025

Linking of some unit tests fails because linker rules for internal rules are missing, which becomes visible with un-inlined symbols. I'll fix that (tomorrow).

Turns out Crimson compiles many of the "common" sources twice: once for libceph-common.a and again for libcrimson-common.a. That alone looks suspicious. But ceph_fs.cc was not included in the latter, and that has caused the linker failure after I moved a symbol to that source. Should be fixed now.

@MaxKellermann
Copy link
Member Author

MaxKellermann commented Apr 25, 2025

jenkins test make check

@MaxKellermann
Copy link
Member Author

MaxKellermann commented Apr 25, 2025

jenkins test make check arm64

1 similar comment
@MaxKellermann
Copy link
Member Author

MaxKellermann commented Apr 25, 2025

jenkins test make check arm64

@cyx1231st
Copy link
Member

cyx1231st commented May 6, 2025

Turns out Crimson compiles many of the "common" sources twice: once for libceph-common.a and again for libcrimson-common.a. That alone looks suspicious. But ceph_fs.cc was not included in the latter, and that has caused the linker failure after I moved a symbol to that source. Should be fixed now.

CC @Matan-B

This is because Crimson needs a special version of the common library (see the macro WITH_CRIMSON). But IIRC this is only related to (dependent by) the crimson subdirectory and irrelavent to ceph fs.

@Matan-B
Copy link
Contributor

Matan-B commented May 6, 2025

Turns out Crimson compiles many of the "common" sources twice: once for libceph-common.a and again for libcrimson-common.a. That alone looks suspicious. But ceph_fs.cc was not included in the latter, and that has caused the linker failure after I moved a symbol to that source. Should be fixed now.

CC @Matan-B

This is because Crimson needs a special version of the common library (see the macro WITH_CRIMSON). But IIRC this is only related to (dependent by) the crimson subdirectory and irrelavent to ceph fs.

Can we leave Crimson cmakes out of this cleanup? FS is not yet supported with Crimson and I expect this won't be the last issue.

@MaxKellermann
Copy link
Member Author

MaxKellermann commented May 6, 2025

Can we leave Crimson cmakes out of this cleanup? FS is not yet supported with Crimson and I expect this won't be the last issue.

I wish, but leaving it out would lead to the mentioned linker failures, because Crimson builds do depend on these symbols. Do you have a different idea how to solve these linker failures?

@github-actions
Copy link

github-actions bot commented Jul 5, 2025

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@github-actions github-actions bot added the stale label Jul 5, 2025
@MaxKellermann
Copy link
Member Author

MaxKellermann commented Jul 9, 2025

Posting a useless comment to prevent the bot from closing this PR.

@github-actions github-actions bot removed the stale label Jul 9, 2025
@vshankar
Copy link
Contributor

vshankar commented Jul 10, 2025

This PR is under test in https://tracker.ceph.com/issues/72073.

vshankar
vshankar reviewed Jul 14, 2025
View reviewed changes
src/include/cephfs/keys_and_values.h
@@ -0,0 +1,39 @@
// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-
Copy link
Contributor

@vshankar vshankar Jul 14, 2025

Choose a reason for hiding this comment

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

@MaxKellermann - this is causing centos9 packaging failures. See: https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=x86_64,AVAILABLE_ARCH=x86_64,AVAILABLE_DIST=centos9,DIST=centos9,MACHINE_SIZE=gigantic/92596//consoleFull

Checking for unpackaged file(s): /usr/lib/rpm/check-files /home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos9/DIST/centos9/MACHINE_SIZE/gigantic/release/20.3.0-1592-g8eaa3dd6/rpm/el9/BUILDROOT/ceph-20.3.0-1592.g8eaa3dd6.el9.x86_64
error: Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/dump.h
   /usr/include/cephfs/json.h
   /usr/include/cephfs/keys_and_values.h

vshankar
vshankar requested changes Jul 14, 2025
View reviewed changes
Copy link
Contributor

@vshankar vshankar left a comment

Choose a reason for hiding this comment

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

@MaxKellermann FYI

@github-actions
Copy link

github-actions bot commented Jul 16, 2025

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

MaxKellermann and others added 11 commits July 22, 2025 10:41
@MaxKellermann @vshankar
include/fs_types: add missing includes
9136ed3 
Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
@MaxKellermann @vshankar
libcephfs: add missing include
09adf8f 
Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
@MaxKellermann @vshankar
include/cephfs/types.h: include cleanup
308f3be 
Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
@MaxKellermann @vshankar
include/cephfs/types.h: move struct keys_and_values to separate header
c914920 
This struct is only used in two sources and moving it to a separate
header means we can eliminate the heavy header dependency on
boost::spirit.

Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
@MaxKellermann @vshankar
include/cephfs/types.h: move JSON methods to json.h
8244685 
This eliminates the heavy header dependency on ceph_json.h from most
includers.

Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
@MaxKellermann @vshankar
common/ceph_fs: include cleanup
8b822b3 
Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
@MaxKellermann @vshankar
include/ceph_fs: un-inline encoding functions
a56aede 
Reduce header dependencies.

Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
@MaxKellermann @vshankar
include/fs_types: un-inline inodeno_t::dump()
7ed0de7 
Allows forward-declaring ceph::Formatter.

Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
@MaxKellermann @vshankar
include/fs_types: un-inline operator<<(inodeno_t)
4d7941f 
Allows switching to `iosfwd`.

Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
@MaxKellermann @vshankar
include/cephfs/types.h: move dump() methods to dump.h
5c6f9d8 
This eliminates the header dependency on Formatter.h from most
includers.

Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
@vshankar
cephfs: add untracked files to ceph.spec.in and debian installs
3679175 
Signed-off-by: Venky Shankar <vshankar@redhat.com>
@vshankar
Copy link
Contributor

vshankar commented Jul 22, 2025

rebased + centos packaging fixes

@vshankar
Copy link
Contributor

vshankar commented Jul 22, 2025

https://shaman.ceph.com/builds/ceph/wip-cephfs-includes/

@vshankar
Copy link
Contributor

vshankar commented Jul 22, 2025

jenkins test api

@vshankar
Copy link
Contributor

vshankar commented Jul 22, 2025

This PR is under test in https://tracker.ceph.com/issues/72244.

vshankar
vshankar approved these changes Aug 4, 2025
View reviewed changes
Copy link
Contributor

@vshankar vshankar left a comment

Choose a reason for hiding this comment

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

https://tracker.ceph.com/projects/cephfs/wiki/QA_main_2025#wip-vshankar-testing-20250730064735-debug

@vshankar vshankar merged commit bde5ae9 into ceph:main Aug 4, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vshankar vshankar vshankar approved these changes

Assignees

No one assigned

Labels

build/ops cephfs Ceph File System common core crimson mon

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@MaxKellermann @cyx1231st @Matan-B @vshankar