Skip to content

cmake: fix the build on AArch64#10427

Closed
tone-zhang wants to merge 0 commit intoceph:masterfrom
tone-zhang:tone_cmake1
Closed

cmake: fix the build on AArch64#10427
tone-zhang wants to merge 0 commit intoceph:masterfrom
tone-zhang:tone_cmake1

Conversation

@tone-zhang
Copy link
Contributor

The PR comes from #10311, because the origin one is in a mess.

Reconstruct the cmake files, extrace one common module from src/CMakeLists.txt.
And update the related CMakeLists.txt files.

When compile the Ceph UT, because the -msse and -msse2 are hardcoded in
the test/CMakeLists.txt, the Ceph UT build fail in AArm64. Fix the issue.

Signed-off-by: tone.zhang tone.zhang@linaro.org

@tchaikov
Copy link
Contributor

@tone-zhang you might be interested in this change. 7818d91

it's what we've been talking about.

@tone-zhang
Copy link
Contributor Author

@tchaikov Kefu, yes, it is the update in rocksdb which we need. Thanks.

@tchaikov
Copy link
Contributor

this change looks good to me, @alimaredia what do you think?

@alimaredia
Copy link
Contributor

alimaredia commented Jul 26, 2016

@dmick @cbodley. Any thoughts?

target_link_libraries(ec_jerasure_sse3 crush ${EXTRALIBS})
set_target_properties(ec_jerasure_sse3 PROPERTIES VERSION 2.0.0 SOVERSION 2
COMPILE_FLAGS ${SSE3_FLAGS})
COMPILE_FLAGS ${COMPILE_TARGETS_FLAGS})
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't look right to me. we only want to pass supported sse3 flags for this target, not all supported flags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cbodley Casey, thanks, I will roll back the code and use sse3 flags as origin.

@cbodley
Copy link
Contributor

cbodley commented Jul 26, 2016

I like how you moved the detection stuff into a module, but we need to keep the flags separated instead of combining them all into COMPILE_TARGETS_FLAGS

@dmick dmick mentioned this pull request Jul 26, 2016
@tone-zhang tone-zhang force-pushed the tone_cmake1 branch 2 times, most recently from 02453ed to cd500c5 Compare July 27, 2016 06:58
@tchaikov
Copy link
Contributor

tchaikov commented Jul 27, 2016

@tone-zhang #10438 is competing with your PR, and it got merged first. if you'd like to continue working on this PR, we can still have the first commit for refactoring the compiler detection.

@tone-zhang
Copy link
Contributor Author

@tchaikov Kefu, I will continue on the first. Thanks!

@wjwithagen
Copy link
Contributor

@tone-zhang
Just a personal note.

From my personal experience with Pulls this large is that that is not going to work.
I once had a pull that also had something like 70+ commits, even 30 file pulls did not work: #7515
So I just closed those.

There is no ill will with the commiters, but the code is mostly changing so rapidly that with 72 files being modified in one pull there is an almost 100% chance that your pull has something to discuss about.
And as a consequence of that the whole of the pull grows old and outdated. And keeping it up to date with HEAD is going to be more an more cumbersome.
@tchaikov is very helpfull in getting me along, but in the end the size of the commit is not going to work. (It only works when you have commit right yourself, and really know what you are doing)

So I personally have moved to smaller commits, with less files changed in one run. Chances of getting such a pull commited are way much bigger. And in the end you will get more commited.
I even make commit of one or 2 lines, 1 2 files. The impact is whay much less. The change is easier to review, and as a consequence is easier approved.

Just my 2 cts.

@tone-zhang
Copy link
Contributor Author

@wjwithagen Willem, it is my fault. I just commit two files I updated, but contain so many non-related commits which are the delta between my working branch and ceph/master. I need more practice to familiar with git commands. Apologize for the mess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants