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

cmake: Support extended attributes when built with cmake #1176

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@jay
Member

jay commented Dec 27, 2016

Several months ago a patch was proposed on the mailing list to support xattr in cmake builds, but it appears to have slipped through the cracks. It is now the commit in this PR. Can a dev familiar with cmake please review?

/cc @sburford @Lekensteyn @snikulov @jzakrzewski

cmake: Support extended attributes when built with cmake
Extended attribute support (--xattr) was omitted when curl was built
with cmake since cmake does not test for and set the HAVE_FSETXATTR
defines.

The configure.ac, configure, make build system is not affected by this
issue.

Closes #XXXX

@jay jay added the cmake label Dec 27, 2016

@mention-bot

This comment has been minimized.

mention-bot commented Dec 27, 2016

@jay, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Sukender, @billhoffman and @Lekensteyn to be potential reviewers.

@Lekensteyn

Changes look good, but see some inline stylistic comments.

if(CURL_XATTR)
check_symbol_exists(fsetxattr "${CURL_INCLUDES}" HAVE_FSETXATTR)
if(HAVE_FSETXATTR)
add_definitions(-DHAVE_FSETXATTR)

This comment has been minimized.

@Lekensteyn

Lekensteyn Jan 3, 2017

Member

This adds an option to the compile command. An alternative is to define macros in a config.h file (lib/curl_config.h.cmake):

#cmakedefine HAVE_FSETXATTR 1

I think this is closer to what autotools does via AC_DEFINE (but I did not check the output).

This comment has been minimized.

@SeanBurford

SeanBurford Jan 12, 2017

Yes, this is a great suggestion.

endforeach(CURL_TEST)
if(HAVE_FSETXATTR_5)
set_source_files_properties(tool_xattr.c PROPERTIES
COMPILE_FLAGS "-DHAVE_FSETXATTR_5")

This comment has been minimized.

@Lekensteyn

Lekensteyn Jan 3, 2017

Member

This can possibly override other compile flags, so try appending properties and use the special property COMPILE_DEFINITIONS:

set_property(SOURCE tool_xattr.c APPEND PROPERTY
  COMPILE_DEFINITIONS HAVE_FSETXATTR_5)

Same comment for the next line. Alternatively, try adding a #cmakedefine as before, autotools uses an AC_DEFINE for these ..._5 and ..._6 options too.

This comment has been minimized.

@SeanBurford

SeanBurford Jan 12, 2017

With the curl_config.h.cmake #cmakedefine suggestion this block becomes much simpler:
+check_symbol_exists(fsetxattr "${CURL_INCLUDES}" HAVE_FSETXATTR)
+if(HAVE_FSETXATTR)

  • foreach(CURL_TEST HAVE_FSETXATTR_5 HAVE_FSETXATTR_6)
  • curl_internal_test_run(${CURL_TEST})
  • endforeach(CURL_TEST)
    +endif(HAVE_FSETXATTR)
@SeanBurford

This comment has been minimized.

SeanBurford commented Jan 9, 2017

Thanks for the feedback, I'll prepare an updated patch.

@SeanBurford

This comment has been minimized.

SeanBurford commented Jan 12, 2017

Thanks again for the feedback. I have attached a shorter patch based on this one that enables xattr support when built using cmake.

cmake-xattr.patch.txt

@jay

This comment has been minimized.

Member

jay commented Jan 12, 2017

cmake-xattr.patch.txt

I've added this patch to the branch as draft2 to make it easier to review

}
#endif
#ifdef HAVE_FSETXATTR_6

This comment has been minimized.

@Lekensteyn

Lekensteyn Jan 12, 2017

Member

Is there any difference between the two main functions below and up?

This comment has been minimized.

@bagder

bagder Jan 12, 2017

Member

5 vs 6 arguments in the fsetxattr() call.

This comment has been minimized.

@Lekensteyn

Lekensteyn Jan 12, 2017

Member

That is for _5 vs _6, but if my diff tool is not broken, the same test programs are duplicated. I think lines 552-568 can be deleted

}
#endif
#ifdef HAVE_FSETXATTR_6

This comment has been minimized.

@Lekensteyn

Lekensteyn Jan 12, 2017

Member

That is for _5 vs _6, but if my diff tool is not broken, the same test programs are duplicated. I think lines 552-568 can be deleted

@snikulov

This comment has been minimized.

Member

snikulov commented Jan 12, 2017

@Lekensteyn You tool is not broken. It is duplicated block at the end of this file.

cmake: Support extended attributes when built with cmake. draft2
Squash me into previous draft.

- Changes to address Peter's review of draft1.

@jay jay force-pushed the jay:cmake_support_xattr branch from fc60641 to 9c05a8b Jan 12, 2017

@jay

This comment has been minimized.

Member

jay commented Jan 12, 2017

It is duplicated block at the end of this file.

my fault, patch applied it even though there was no change from the first draft. fixed, please check again

@Lekensteyn

This comment has been minimized.

Member

Lekensteyn commented Jan 12, 2017

LGTM, let's see what the buildbots say

@jay jay closed this in 1c877a0 Feb 1, 2017

@jay jay deleted the jay:cmake_support_xattr branch Feb 1, 2017

@jay

This comment has been minimized.

Member

jay commented Feb 1, 2017

Just landed, thanks guys

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