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: several code cleanups #4975

Closed
wants to merge 1 commit into from
Closed

CMake: several code cleanups #4975

wants to merge 1 commit into from

Conversation

@DerDakon
Copy link
Contributor

@DerDakon DerDakon commented Feb 24, 2020

I've build curl successfully with 3.0.2 and 3.16.2 afterwards.

@bagder bagder added the cmake label Feb 24, 2020
@DerDakon DerDakon force-pushed the emlix:cmake branch 3 times, most recently from 60400b6 to 280379a Feb 25, 2020
@DerDakon DerDakon requested review from bagder, Lekensteyn and MarcelRaad Feb 25, 2020
Copy link
Member

@bagder bagder left a comment

I don't know cmake enough to review these changes.

@jay
Copy link
Member

@jay jay commented Feb 26, 2020

Maybe @jzakrzewski or @Lekensteyn will know? I'm fine with it as long as it doesn't bump the minimum version required.

Copy link
Member

@Lekensteyn Lekensteyn left a comment

Looks good for the most part, I've suggested some changes.

@bagder Each individual commit message is helpful. How should a merge be handled? By merging it as-is, by squashing, or other ways?

CMake/FindNSS.cmake Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@bagder
Copy link
Member

@bagder bagder commented Feb 27, 2020

This PR is currently at 13 commits but clearly some of the commits fixes earlier commits in the series itself, and we generally frown upon that. They should be reduced to a set of separate commits that each have their own commit message and reasoning.

If that's too much work, I would rather squash them into a single commit.

@DerDakon DerDakon force-pushed the emlix:cmake branch 2 times, most recently from 7874c75 to c6a1668 Feb 27, 2020
Copy link
Member

@Lekensteyn Lekensteyn left a comment

Looks good, @DerDakon could you squash the changes in a smaller commit?

@DerDakon DerDakon force-pushed the emlix:cmake branch 2 times, most recently from cb05285 to 8b45f0c Feb 27, 2020
@Lekensteyn
Copy link
Member

@Lekensteyn Lekensteyn commented Feb 27, 2020

I think that all CMake changes could be squashed into a single commit, "CMake: cleanup and simplify" or similar. You can keep the current subjects as bullet points in the description if desired. How does that sound?

Edit: additionally, please rebase this on master. It looks like the unbuntu typo was fixed in #4866.

-remove check for unsupported old CMake versions

-not link to c-ares library twice

-modernize custom Find modules

 FindLibSSH2:
   -pass version to FPHSA to show it in the output
   -use LIBSSH2_VERSION define to extract the version number in one shot. This
    variable exists in the header for 10 years.
   -remove unneeded code

 FindNGHTTP2.cmake
   -drop needless FPHSA argument
   -mark found variables as advanced

 FindNSS.cmake:
   -show version number

 FindCARES.cmake:
   -drop default paths
   -use FPHSA instead of checking things by hand

-remove needless explict variable dereference

-simplify count_true()

-allow all policies up to version 3.16 to be set to NEW

-do not rerun check for -Wstrict-aliasing=3 every time

 In contrast to every other compiler flag this has a = in it, which CMake can't
 have in a variable name.

-only read the interesting strings from curlver.h
@DerDakon DerDakon force-pushed the emlix:cmake branch from 8b45f0c to 69334d8 Feb 28, 2020
CMakeLists.txt Show resolved Hide resolved
@jay jay closed this in fc9312f Mar 1, 2020
@jay
Copy link
Member

@jay jay commented Mar 1, 2020

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.