-
Notifications
You must be signed in to change notification settings - Fork 104
build flavors: Cleanup "crimson", introduce "debug" #2497
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,6 +86,17 @@ else | |
| printf 'No cmake debug options added to branch %s.\n' "$BRANCH" | ||
| fi | ||
|
|
||
| # Tentacle is the last release that needs dedicated Crimson builds, | ||
| # Later releases are able to use Crimson with the default build. | ||
| # As the "Crimson flavor" is no longer available, we need a *temporary* way | ||
| # to be able build Crimson for tentacle. | ||
| # Note: This could be removed once Crimson we have Umbrella release builds. | ||
| if [[ "$BRANCH" == *-crimson-tentacle ]]; then | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's have Alternatively: why not just turn these switches on unconditionally for tentacle?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I guess the -debug suffix could be removed with the new "debug" flavor introduced. @batrick, WDYT? I'll try moving this logic to git trailers instead of branch name suffix
Unless I misunderstand the suggestion, this way every Tentacle build will be with Crimson enabled. We only want to be able to enable Crimson with tentacle iff specifically needed for some reason.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah, I missed that. That sounds even better!
That would be great. I've tried to make it easier to build branches with these new git trailers here: Please give it a try! Maybe you can submit a change for the new git trailer?
Got it, nevermind.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @batrick Are these suggestions 'nice to haves' or must haves? I think CentOS9 builds are blocked by this PR not being merged.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice to haves |
||
| CEPH_EXTRA_RPMBUILD_ARGS="--with crimson" | ||
| CEPH_EXTRA_CMAKE_ARGS+=" -DWITH_CRIMSON=true" | ||
| printf 'Added Crimson Tentacle cmake configs to branch %s. CEPH_EXTRA_CMAKE_ARGS: %s\n' "$BRANCH" "$CEPH_EXTRA_CMAKE_ARGS" | ||
| fi | ||
|
|
||
| ceph_build_args_from_flavor ${FLAVOR} | ||
|
|
||
| mkdir -p release | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will still need to be able to build Tentacle until upgrade testing from T to V is no longer necessary (probably when V is EOL).
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is only true for Crimson. We only build the last stable release (and main) as there's no upgrade testing/support yet. Starting U we will probably start working on upgradability. We still break on-disk data structures from time to time as we are still flexible with this - the plan is to stop with this approach starting U.
This is why we should be able to remove this bit once U is out as Crimson will no longer be supported in T at all. I'll mention this in this comment to avoid confusion.