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

Changes to build on an M1 mac #14

Closed
wants to merge 2 commits into from

Conversation

leonid-s-usov
Copy link

@leonid-s-usov leonid-s-usov commented Jul 25, 2023

It appears as if this change is pushing 2 commits, including the 50c6790, but that's just because the commit hasn't been pushed to the master while it's currently the one recorded in the ceph/main as the submodule commit:

$git -P show origin/main -s
commit 38dea7dc2156bd003d2291e9f2d82688490beb53 (origin/main, main)
Merge: f82b9942d6d ad1202be685
Author: zdover23 <zac.dover@proton.me>
Date:   Wed Jul 26 09:07:40 2023 +1000

    Merge pull request #52630 from zdover23/wip-doc-2023-07-25-readmemd-1-of-x

    doc: update README.md

    Reviewed-by: Anthony D'Atri <anthony.datri@gmail.com>

$git ls-tree origin/main:src | grep seastar
160000 commit 50c6790df3db6ab38b5fea7e03494828dc2aafdc	seastar

This PR is supporting ceph/ceph#52629

tchaikov and others added 2 commits February 2, 2023 10:17
Ceph is stilling using errorated future with handle_exception(), but
errorated future is not a seastar::Futuer<>. so we need to revert this
change at this moment.

This reverts commit 955c584.
Copy link

@athanatos athanatos left a comment

Choose a reason for hiding this comment

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

We generally get changes merged upstream -- the seastar upstream is generally very receptive. I'd think the librt change would be pretty easy to convince them of. I don't understand why the dpdk change is needed though.

.gitmodules Outdated
@@ -1,3 +1,3 @@
[submodule "dpdk"]
path = dpdk
url = ../dpdk
url = https://github.com/ceph/dpdk

Choose a reason for hiding this comment

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

Why is this part necessary?

Copy link
Author

Choose a reason for hiding this comment

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

this was needed for my local version since I didn't want to fork that submodule, too, just for the sake of the relative URL to work.
However, this PR itself doesn't require the change, so I can revert it

Copy link
Author

Choose a reason for hiding this comment

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

it's a separate commit, actually, so apparently I just pushed too much for the PR. I'll exclude that commit simply

Copy link
Author

Choose a reason for hiding this comment

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

done

@leonid-s-usov
Copy link
Author

We generally get changes merged upstream -- the seastar upstream is generally very receptive.

@athanatos my concern is whether it will be then acceptable to update the ceph main branch to whatever upstream head with this PR merged there. Or do you mean that once the change is accepted by the upstream we can then safely merge it locally over whatever version we currently have referenced?

@athanatos
Copy link

We tend to rebase the ceph seastar branch whenever seastar upstream has something we want -- this includes anytime we get something we want merged. We should probably also do it every month or something, but it hasn't been a pressing issue. Anyway, once your change merges it'll be easy to rebase/update our submodule.

@leonid-s-usov
Copy link
Author

Following the investigation done in scylladb#1806 this PR can be closed as incomplete

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.

3 participants