Skip to content

set version for project() and add CPack support - #15281

Closed
zjyhjqs wants to merge 5 commits into
curl:masterfrom
zjyhjqs:feat/cpack
Closed

set version for project() and add CPack support#15281
zjyhjqs wants to merge 5 commits into
curl:masterfrom
zjyhjqs:feat/cpack

Conversation

@zjyhjqs

@zjyhjqs zjyhjqs commented Oct 13, 2024

Copy link
Copy Markdown
Contributor

Note: the version like 8.11.0-DEV is not a valid version for project(), so need to extract the major, minor and patch parts.

Comment thread CMakeLists.txt Outdated
Comment thread CMakeLists.txt Outdated

@vszakats vszakats Oct 13, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suggest renaming variables in a separate PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've separated it into a commit. If that's not enough I can create a new PR.

@vszakats vszakats Oct 13, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd suggest a separate PR. That said, what is the reason to rename this variable? It can be argued it's both curl and libcurl version, and since it has been CURL_ for a long time, why change now?

edit: CURL_ is also the namespace we use for curl-specific CMake variables. (And LIBCURL_PC_ but that is a separate, libcurl-specific namespace.) TL;DR: existing name looks fine to me.

@zjyhjqs zjyhjqs Oct 13, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

CURL_VERSION will be initialized by project() (without the -DEV as suffix because the syntax rejects). The variables related to project() should be generated by project() IMO, though I'm not insist on it.

Edit: Since CMake 3.30, behaviour of variables generated by project has been changed. Though it doesn't affect <Project>_VERSION* now, I think it'd better not to set CURL_VERSION explicitly.

So, the renaming is related to setting the project(VERSION). Should this be extracted to a PR?

@vszakats vszakats Oct 13, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, so project() is setting the CURL_VERSION and *_MAJOR/MINOR/PATCH variables automatically, but also differently depending on version and/or CMP policy. E.g. cmake 3.30.x is setting them.

Before this patch we already set CURL_VERSION to 8.11.0-DEV. This changes to 8.11.0 when using project().

What is the advantage of using project() for setting the CURL_VERSION variable? Why do we need this?

edit: link for reference: https://cmake.org/cmake/help/latest/command/project.html#options

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

wouldn't we need this to catch non-DEV versions?:

string(REGEX REPLACE "([0-9]+\.[0-9]+)\.([0-9]+.*)" "\\2" CPACK_PACKAGE_VERSION_PATCH "${LIBCURL_VERSION}")

Yes. I've tested both DEV and non-DEV versions. This works fine.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would you want to do the renaming from LIBCURL_VERSION* to _cmake_version*?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

CMAKE_VERSION is the version of CMake. But we could add an underscore prefix (with full lower-case)?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I meant rename to _curl_version_*, sorry.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I meant rename to _curl_version_*, sorry.

done

Comment thread CMakeLists.txt Outdated
Comment thread CMakeLists.txt Outdated
@zjyhjqs
zjyhjqs force-pushed the feat/cpack branch 3 times, most recently from ecf81ec to f348f8e Compare October 13, 2024 15:49
@vszakats

Copy link
Copy Markdown
Member

Thank you @zjyhjqs, this is merged now.

pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
Note: the version like `8.11.0-DEV` is not a valid version for
`project()`, so need to extract the major, minor and patch parts.

Previous, manual, `CURL_VERSION` macro is defined by `project()`
after this patch, so rename existing `CURL_VERSION*` variables to
`_curl_version*`.

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

Development

Successfully merging this pull request may close these issues.

2 participants