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

build: improve OS string in CMake and config-win32.h #9117

Closed
wants to merge 3 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Jul 7, 2022

This patch makes CMake fill the "OS string" with the value
of CMAKE_C_COMPILER_TARGET, if passed. This typically
contains a triplet, the same that is passed to ./configure via
--host=.

For non-CMake, non-autotools, Windows builds, this patch
adds the ability to override the default OS value in
lib/config-win32.h.

With these its possible to get the same OS string across the
three build systems.

This patch supersedes the earlier, partial, CMake-only solution:
435f395, thus retiring the CURL_OS_SUFFIX CMake option.

Closes #xxxx

@vszakats vszakats added the build label Jul 7, 2022
Autotools builds fill the curl "OS string" with the platform triplet.
This value appears in the curl version info:
   `curl 7.84.0 (os_string)`

This patch adds the ability to do the same with CMake and non-autotools
builds, allowing to configure consistent version strings (and thus
reproducible binaries) across build systems.

Add the `CURL_OS` macro and CMake configuration variable to allow
passing a custom "OS string", e.g.:

with CMake:
  `cmake [...] -DCURL_OS=os_string`
with non-autotools builds, via C flag:
  `-DCURL_OS="os_string"`

`os_string` is typically a triplet, e.g.: `x86_64-pc-linux-gnu`

Before this patch CMake uncoditionally filled this value with
`CMAKE_SYSTEM_NAME`, which is a crude platform value, e.g. for Windows
it is always "Windows", without CPU or other information. Now it allows
to override this via its `CURL_OS` configuration option.

For non-autotools builds, it was automatically set to a value in
`lib/config-*.h` headers. Now they allow to override these defaults when
the `CURL_OS` macro is set. (For now this is implemented for
`config-win32.h` only.)

This patch supersedes the earlier, partial, CMake-only solution:
435f395, thus retiring the
`CURL_OS_SUFFIX` CMake option.

Closes #xxxx
@jay
Copy link
Member

jay commented Jul 8, 2022

Do you see anyone besides yourself manually setting the OS string? It seems to me a better solution would be improve cmake's detected OS string.

@vszakats
Copy link
Member Author

vszakats commented Jul 8, 2022

Anybody who wants to create reproducible builds across multiple build system will face these, but yes, it might be only me doing that. Not sure how to improve this in CMake, but even if doable, I suspect any autodetection would be inherently different from autotools results and will definitely remain different from what config-win32.h does. This is also a better fix for CMake than the earlier one already committed.

@dfandrich
Copy link
Contributor

dfandrich commented Jul 8, 2022 via email

@vszakats
Copy link
Member Author

vszakats commented Jul 9, 2022

Actually autotools isn't even guessing the triplet in my (or any cross-build) case, I'm passing it to ./configure via --host=. This value then find its way into the version string.

The same can be done with CMake, where the triplet is passed (for cross-builds at least) via CMAKE_C_COMPILER_TARGET. That can be used to fill OS, if not empty:

set(OPERATING_SYSTEM "${CMAKE_SYSTEM_NAME}")
if(CMAKE_C_COMPILER_TARGET)
  set(OS "\"${CMAKE_C_COMPILER_TARGET}\"")
else()
  set(OS "\"${CMAKE_SYSTEM_NAME}\"")
endif()

This would change the default behaviour for CMake builds where this value is passed (unless we put it behind an extra switch).

@vszakats vszakats changed the title build: add CURL_OS option to set the OS string build: improve OS string in CMake and config-win32.h Jul 9, 2022
@bagder bagder added the cmake label Jul 9, 2022
@jay
Copy link
Member

jay commented Jul 9, 2022

I don't understand how this makes it reproducible in Windows pregenerated config. If the triplet is the same it's already the same string. Similar for cmake, if anything "Windows" is always "Windows"? Not that it should be that, but you can see what I mean. IMO CURL_OS_SUFFIX is a workaround for the actual problem which is lack of cmake detailed target recognition

@vszakats
Copy link
Member Author

vszakats commented Jul 9, 2022

@jay: In case of cross-builds there no automatic target recognition (there cannot be), it's an input provided manually to the build systems by the builder. Currently this input is only used to form the OS string with autotools. Makefile.m32 uses some hard-coded value, and CMake just puts 'Windows' there. It means that even though I do build for the exact same target with the same triplet and with precisely the same build options, the three build systems generate objects that differ only in the object compiled from version.c. (At the moment I need to resort to patching curl to force them use the same OS string.)

In general, CMake's solution to put 'Windows' or other crude OS string there seems wrong in general, especially when someone clearly passed a triplet to it via CMAKE_C_COMPILER_TARGET.

The (updated) proposed solution makes CMake use the above value, if present. It also lets the user override OS in config-win32.h (used by Makefile.m32 and perhaps some MSVC builds.) The latter could be tweaked to produce the desired results automatically (e.g. by recognizing MinGW env). Though with more complexitly, fragility and making it less future-proof.

@jay
Copy link
Member

jay commented Jul 10, 2022

Ok for most recent cmake change. Are you saying you need to override OS in config-win32? What is wrong with the OS logic already there, is it not accurate enough?

edit: Are you saying you want the same OS string for both build systems?

@vszakats
Copy link
Member Author

vszakats commented Jul 10, 2022

@jay: That's correct, I'd like to see the same OS string for CMake, Makefile.m32 (and autotools) builds.

@vszakats vszakats closed this in ca73991 Jul 11, 2022
@vszakats vszakats deleted the curl-os branch July 11, 2022 19:42
vszakats added a commit to curl/curl-for-win that referenced this pull request Jul 11, 2022
Apply upstream patch [1] [2], which unconditionally sets the OS string to
the cross-build target value with CMake. For Makefile.m32, use the new
option to override the OS string to the triplet with an extra build
option.

The internal switch `CW_DEV_CROSSMAKE_REPR` no longer has an effect on
this. And, curl-for-win could get rid of two local (disabled-by-default)
patches.

[1] curl/curl@ca73991
[2] curl/curl#9117
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.

None yet

4 participants