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.

Sign up for GitHub

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

Jump to bottom

changes for Clang and yasm #10417

Merged
merged 3 commits into from Aug 9, 2016
Merged

changes for Clang and yasm #10417

merged 3 commits into from Aug 9, 2016

Conversation

wjwithagen
Copy link
Contributor

@wjwithagen wjwithagen commented Jul 24, 2016 •
edited

  • Clang does not need all flags and options
  • uname -m on FreeBSD returns amd64

@wjwithagen
Copy link
Contributor Author

@tchaikov
Another small set of commits to get Clang and Yasm going.

tchaikov
tchaikov reviewed Jul 26, 2016
View reviewed changes
src/CMakeLists.txt

if(NOT CMAKE_BUILD_TYPE)
set(CMAKE_BUILD_TYPE "RelWithDebInfo" CACHE STRING "Default BUILD_TYPE is RelWithDebInfo, other options are: Debug, Release, and MinSizeRel." FORCE)
endif()

if(NOT CMAKE_BUILD_TYPE STREQUAL Debug)
if(NOT CMAKE_BUILD_TYPE MATCHES "Debug")
Copy link
Contributor

@tchaikov tchaikov Jul 26, 2016 •
edited

Choose a reason for hiding this comment

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

@wjwithagen would you please move this change out of this commit? you can always override the CFLAGS, CXXFLAGS and CMAKE_CXX_FLAGS_DEBUG when launching cmake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tchaikov
I only added the part where we'd like to run debug, and then these flags look rather appropriate.
But you ar right that they could be set from outside of Cmake.

Copy link
Contributor

Choose a reason for hiding this comment

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

could you revert this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

@wjwithagen ping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tchaikov
It should be out by a long time, but I'll check and otehrwise fix

@wjwithagen wjwithagen force-pushed the wip-wjw-freebsd-clang branch 4 times, most recently from 822ba42 to 8ce01d1 Compare July 26, 2016 13:07
tchaikov
tchaikov reviewed Jul 26, 2016
View reviewed changes
src/CMakeLists.txt
set(EXTRA_CXXFLAGS -fPIC -Wno-unused-variable -DNDEBUG)
if(FREEBSD)
# set(EXTRA_CXXFLAGS ${EXTRA_CXXFLAGS} -I/usr/local/include)
set(MAKEFLAGS ${MAKEFLAGS})
Copy link
Contributor

Choose a reason for hiding this comment

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

again, what is this for? it's just a no-op, imo.

@wjwithagen wjwithagen force-pushed the wip-wjw-freebsd-clang branch 2 times, most recently from 8472ac7 to 7a1b044 Compare July 26, 2016 18:34
@wjwithagen wjwithagen mentioned this pull request Jul 27, 2016
Merged
@wjwithagen wjwithagen force-pushed the wip-wjw-freebsd-clang branch 4 times, most recently from d4f98b5 to c31f58e Compare August 1, 2016 14:31
tchaikov
tchaikov reviewed Aug 1, 2016
View reviewed changes
src/CMakeLists.txt
@@ -35,19 +51,21 @@ if(NOT CMAKE_BUILD_TYPE STREQUAL Debug)
CMAKE_C_FLAGS_${build_type_upper})
string(REGEX REPLACE "(^| )[/-]D *NDEBUG($| )" " " "${flags}" "${${flags}}")
endforeach()
endif()
endif(NOT CMAKE_BUILD_TYPE STREQUAL Debug)
Copy link
Contributor

Choose a reason for hiding this comment

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

could you drop unrelated changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tchaikov
I was under the impression that "while there" one is allowed to fix imperfacetions in
code?
But I'll take it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, 1) but i don't think we need to "fix" it. the if block is smaller enough, that we know where it starts. 2) and you are not changing the if block, so i don't really think you were there.

tchaikov
tchaikov reviewed Aug 1, 2016
View reviewed changes
src/CMakeLists.txt
$<TARGET_OBJECTS:krbd_objs>)
endif(LINUX)
endif(WITH_RBD)
if(WITH_KVS)
Copy link
Contributor

Choose a reason for hiding this comment

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

i still don't understand the relation between WITH_KVS and parse_secret_objs. could you enlighten me on it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tchaikov
Could be that I'm misinterpreting things. But the KeyValue store seems to use the file common/secret.c which includes keyutils.h.
And that we don't have in FreeBSD.
And I have not figured out how to patch this up: either write a replacement, and/or stub it up.

I'll take a note of this and put it at least in my freebsd-todo.txt

Copy link
Contributor

@tchaikov tchaikov Aug 2, 2016 •
edited

Choose a reason for hiding this comment

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

But the KeyValue store seems to use the file common/secret.c which includes keyutils.h.

i am not sure how you come to this. secret.c is used by mount.ceph.c and krbd.cc .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On 2-8-2016 09:58, Kefu Chai wrote:

In src/CMakeLists.txt
#10417 (comment):

set(DENCODER_EXTRALIBS
${DENCODER_EXTRALIBS}
rbd_replay_types)
-endif(${WITH_RBD})

  • if(LINUX)
  • list(APPEND dencoder_srcs
  •    $<TARGET_OBJECTS:krbd_objs>)
  • endif(LINUX)
    +endif(WITH_RBD)
    +if(WITH_KVS)
But the KeyValue store seems to use the file common/secret.c which
includes keyutils.h.

|secret.c| is used by |mount.ceph.c| and |krbd.cc| also. i am not sure
how you come to this.

I just looked at the source of secret.c. :)
Perhaps it can be removed, but have not yet dared to do so.
KV exclusion for this also seems to work.

@wjwithagen
Copy link
Contributor Author

@tchaikov
I'm going to rip out the whole commit with the RBD and KVS stuff
And fix the CLANG/yasm part, so we can at least get that one in.
For the RBD and KVS I'll either add it to one of the other pulls, or even make it a seperate one.

@wjwithagen wjwithagen force-pushed the wip-wjw-freebsd-clang branch 3 times, most recently from 1e6eb4a to 9e05d20 Compare August 1, 2016 16:18
@tchaikov
Copy link
Contributor

tchaikov commented Aug 1, 2016

could you address the comments? they are dropped on the floor, i guess.

@wjwithagen
Copy link
Contributor Author

@tchaikov
It is not my intention to drop anything on the floor.
I'm having sort of a fight with git rebase to get ride of a merge confilict.

@wjwithagen
Copy link
Contributor Author

@tchaikov
Failure is in eph_objectstore_tool.py, which seems to be in a real bad state.

tchaikov
tchaikov reviewed Aug 2, 2016
View reviewed changes
src/CMakeLists.txt
set(CMAKE_CXX_FLAGS "${CMAKE_C_FLAGS} -ftemplate-depth-1024 -Wnon-virtual-dtor -Wno-invalid-offsetof -Wstrict-null-sentinel -Woverloaded-virtual")
set(CMAKE_CXX_FLAGS "${CMAKE_C_FLAGS} -ftemplate-depth-1024 -Wno-invalid-offsetof")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wnon-virtual-dtor -Woverloaded-virtual")
if(CMAKE_CXX_COMPILER_ID MATCHES "GNU")
Copy link
Contributor

Choose a reason for hiding this comment

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

would be better if we can be consistent regarding to the use of MATCHES "GNU" and STREQUAL GNU?

@tchaikov
Copy link
Contributor

tchaikov commented Aug 2, 2016 •
edited

please ping me once all comments addressed, thanks.

@wjwithagen
src/CmakeLists.txt: Changes for the Clang compiler
59c208d 
 - Added a lot more Clang flags to supress warnings, since Clang
   is way much more verbose. And while there rearranged the layout.
 - Move Linking flags to correct Cmake variables, so Clang does not
   complain about flags that are not appropriate
 - Clang/Cmake debugging: use CMAKE_CXX_FLAGS_DEBUG in buildscript
 - Move Linux specifics to if-endif blocks
   - -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
@wjwithagen
src/CmakeLists.txt: Changes for the arch selctor with yasm compiler
8ba3eaa 
 - uname onf freebsd returns amd64

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
@wjwithagen
src/CmakeLists.txt: Changes Linux type include libraries
b175a86 
 - On FreeBSD libuuid, libresolv, libdl are all in libc.
   So do not use them while linking, otherwise missing lib errors
   will result
 - libdl is sorted out by stdard CMake functionality

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
@wjwithagen
Copy link
Contributor Author

@tchaikov
Ping

@tchaikov
Copy link
Contributor

tchaikov commented Aug 2, 2016

lgtm.

@wjwithagen
Copy link
Contributor Author

@tchaikov
This has been in this state for about 7 days.
Any particulars we are waiting for?

@tchaikov
Copy link
Contributor

tchaikov commented Aug 9, 2016

@wjwithagen lemme push this change to gb to get it tested. then i guess it's good to merge.

@tchaikov
Copy link
Contributor

tchaikov commented Aug 9, 2016

pushed to wip-kefu-testing.

@tchaikov tchaikov merged commit 14f105c into ceph:master Aug 9, 2016
@wjwithagen
Copy link
Contributor Author

@tchaikov
Thanx for getting this in. Up to the next ones.....

@wjwithagen wjwithagen deleted the wip-wjw-freebsd-clang branch August 9, 2016 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
build/ops needs-qa
Projects
None yet
Milestone
No milestone
2 participants
@wjwithagen @tchaikov