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

cmake: Fix build with GSSAPI #3744

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
3 participants
@jzakrzewski
Copy link
Contributor

commented Apr 7, 2019

This primarily fixes #3743

@jzakrzewski jzakrzewski requested a review from snikulov Apr 7, 2019

@jzakrzewski

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

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

@jay

This comment has been minimized.

Copy link
Member

commented Apr 7, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

@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

This comment has been minimized.

Copy link
Member

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
Copy link
Member

left a comment

LGTM

@jzakrzewski

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

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

This comment has been minimized.

Copy link
Member

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 jzakrzewski force-pushed the jzakrzewski:cmake/gss branch from 0ef92e8 to 136f518 Apr 8, 2019

jzakrzewski added a commit to jzakrzewski/curl that referenced this pull request Apr 8, 2019

cmake: minor cleanup
- 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

cmake: avoid linking executable for some tests with cmake 3.6+
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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

Now it's hopefully nicer :)

@jay

This comment has been minimized.

Copy link
Member

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 added some commits Apr 7, 2019

cmake: minor cleanup
- 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
cmake: avoid linking executable for some tests with cmake 3.6+
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
cmake: clear CMAKE_REQUIRED_LIBRARIES after each use
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 #3743
closes #3744

@jzakrzewski jzakrzewski force-pushed the jzakrzewski:cmake/gss branch from 136f518 to 075c67b Apr 9, 2019

@jay

This comment has been minimized.

Copy link
Member

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

cmake: minor cleanup
- 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

cmake: avoid linking executable for some tests with cmake 3.6+
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 jzakrzewski:cmake/gss branch Apr 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.