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

cmake: Fix build with GSSAPI #3744

Closed
wants to merge 3 commits into from
Closed

cmake: Fix build with GSSAPI #3744

wants to merge 3 commits into from

Conversation

jzakrzewski
Copy link
Contributor

This primarily fixes #3743

@jzakrzewski
Copy link
Contributor Author

The coverage decrease seems to be false. Only CMake files changed and the coverage build is done with the autotools...

@jay
Copy link
Member

jay commented Apr 7, 2019 •
edited

I suggest squash

cmake: Fix build with GSSAPI

- Remove unneeded include_regular_expression.

- Don't check for pre-3.0.0 CMake version.

We already require at least 3.0.0, so it's just clutter.

- etc

- etc

Closes #xxxx

@jzakrzewski
Copy link
Contributor Author

@jay If that's how you want to have it - no problem. I was just trying to make one thing per commit (if not for the clear history, then at least for review).
I can squash it in the evening.

@snikulov
Copy link
Member

snikulov commented Apr 8, 2019

@jzakrzewski you've just added more changes then required to solve #3743 :)
For me it's OK.
So go ahead if @jay have no objections.

snikulov
snikulov approved these changes Apr 8, 2019
View reviewed changes
Copy link
Member

@snikulov snikulov left a comment

Choose a reason for hiding this comment

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

LGTM

@jzakrzewski
Copy link
Contributor Author

Yeah, I've sneaked in some simple cleanup and my initial (but insufficient) solution using the CMAKE_TRY_COMPILE_TARGET_TYPE. The fixing commit is clearly marked though.
Personally I'd squash first 3 together and let others as-is. But in the end I don't really care that much and will do however the more experienced colleagues find better.

@jay
Copy link
Member

jay commented Apr 8, 2019

Personally I'd squash first 3 together and let others as-is.

Anything you want to leave separate please add Ref: lines since they came from this PR.

@snikulov snikulov added the cmake label Apr 8, 2019
jzakrzewski added a commit to jzakrzewski/curl that referenced this pull request Apr 8, 2019
@jzakrzewski
cmake: minor cleanup
35ed783 
- unneeded include_regular_expression
  It was setting what is already a default

- remove duplicated include

- don't check for pre-3.0.0 CMake version
  We already require at least 3.0.0, so it's just clutter

Ref: curl#3744
jzakrzewski added a commit to jzakrzewski/curl that referenced this pull request Apr 8, 2019
@jzakrzewski
cmake: avoid linking executable for some tests with cmake 3.6+
16efa5a 
With CMAKE_TRY_COMPILE_TARGET_TYPE set to STATIC_LIBRARY, the try_compile()
(which is used by check_c_source_compiles()) will build static library
instead of executable. This avoids linking additional libraries in and thus
speeds up those checks a little.
Would also avoid curl#3743 on itself with cmake 3.6 or above.

Ref: curl#3744
jzakrzewski added a commit to jzakrzewski/curl that referenced this pull request Apr 8, 2019
@jzakrzewski
cmake: clear CMAKE_REQUIRED_LIBRARIES after each use
136f518 
fixes curl#3743
closes curl#3744
@jzakrzewski
Copy link
Contributor Author

Now it's hopefully nicer :)

@jay
Copy link
Member

jay commented Apr 8, 2019

Yes it's better but I have some formatting suggestions for each:


cmake: minor cleanup indent without line break after bullet point may be autowrapped and treated as continuing a sentence. Please use periods in such a case and/or a line break.

- abcdefg

- qwerty.

- Remove unneeded include_regular_expression.

It was setting what is already a default.

- Remove unneeded include_regular_expression.

  It was setting what is already a default.

- Remove unneeded include_regular_expression.
  It was setting what is already a default.

cmake: clear CMAKE_REQUIRED_LIBRARIES after each use needs the purpose which is to fix the GSSAPI build.How you might do that:

cmake: Fix GSSAPI build

- clear CMAKE_REQUIRED_LIBRARIES after each use.

fixes #3743
closes #3744

cmake: clear CMAKE_REQUIRED_LIBRARIES after each use

This fixes the GSSAPI build which failed when checking for recv. Error
log reveals that it tries to link the Kerberos libraries, which is
totally unnecessary.

fixes #3743
closes #3744

cmake: avoid linking executable for some tests with cmake 3.6+ needs line break between paragraphs and what it fixes, for example:

This commit also avoids #3743 (GSSAPI build errors) on itself with cmake
3.6 or above. That issue was fixed separately for all versions.

@jzakrzewski
cmake: minor cleanup
7f49c7a 
- Remove nneeded include_regular_expression.
  It was setting what is already a default.

- Remove duplicated include.

- Don't check for pre-3.0.0 CMake version.
  We already require at least 3.0.0, so it's just clutter.

Ref: curl#3744
@jzakrzewski
cmake: avoid linking executable for some tests with cmake 3.6+
c41d9b6 
With CMAKE_TRY_COMPILE_TARGET_TYPE set to STATIC_LIBRARY, the try_compile()
(which is used by check_c_source_compiles()) will build static library
instead of executable. This avoids linking additional libraries in and thus
speeds up those checks a little.

This commit also avoids curl#3743 (GSSAPI build errors) on itself with cmake
3.6 or above. That issue was fixed separately for all versions.

Ref: curl#3744
@jzakrzewski
cmake: clear CMAKE_REQUIRED_LIBRARIES after each use
075c67b 
This fixes GSSAPI builds with the libraries in a non-standard location.
The testing for recv() were failing because it failed to link
the Kerberos libraries, which are not needed for this or subsequent
tests.

fixes curl#3743
closes curl#3744
@jay
Copy link
Member

jay commented Apr 9, 2019

Looking good. All the CI failures appear due to transient stalls and can be ignored. The coveralls 7% reduction in coverage doesn't make any sense to me.

jzakrzewski added a commit that referenced this pull request Apr 10, 2019
@jzakrzewski
cmake: minor cleanup
639cfeb 
- Remove nneeded include_regular_expression.
  It was setting what is already a default.

- Remove duplicated include.

- Don't check for pre-3.0.0 CMake version.
  We already require at least 3.0.0, so it's just clutter.

Ref: #3744
jzakrzewski added a commit that referenced this pull request Apr 10, 2019
@jzakrzewski
cmake: avoid linking executable for some tests with cmake 3.6+
be17f29 
With CMAKE_TRY_COMPILE_TARGET_TYPE set to STATIC_LIBRARY, the try_compile()
(which is used by check_c_source_compiles()) will build static library
instead of executable. This avoids linking additional libraries in and thus
speeds up those checks a little.

This commit also avoids #3743 (GSSAPI build errors) on itself with cmake
3.6 or above. That issue was fixed separately for all versions.

Ref: #3744
@jzakrzewski jzakrzewski deleted the cmake/gss branch April 10, 2019 17:17
@lock lock bot locked as resolved and limited conversation to collaborators Jul 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@snikulov snikulov snikulov approved these changes

Assignees
No one assigned
Labels
cmake
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CMake: configure may fail when building with custom GSSAPI build
3 participants
@jzakrzewski @jay @snikulov