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

Update uvwasi CMakeLists.txt #276

Merged
merged 1 commit into from
Jul 26, 2024
Merged

Conversation

kulcsaradam
Copy link
Contributor

@kulcsaradam kulcsaradam commented Jul 24, 2024

FetchContent_Populate is deprecated, updated the cmake file to use FetchContent_MakeAvailable.
Also updated minimum cmake version for FetchContent_MakeAvailable compatibility.
Also fix a bug where the flag '-m32' was not added when building in 32
bit mode.

@kulcsaradam
Copy link
Contributor Author

The function FetchContent_Populate is being deprecated and should no longer be used according to Cmake. For reference: https://cmake.org/cmake/help/latest/policy/CMP0169.html#policy:CMP0169

It should be replaced by FetchContent_MakeAvailable, which was introduced in version 3.14, thus, if we replace FetchContent_Populate the miminum required cmake version should be updated too.

It is unclear when FetchContent_Populate or it's behavior will be removed or changed.

@@ -29,7 +29,7 @@ if(CMAKE_C_COMPILER_ID MATCHES "AppleClang|Clang|GNU")

if (UVWASI_BUILD_X86)
# set x86 build flag
add_compile_options( -m32 )
set (CMAKE_C_FLAGS ${CMAKE_C_FLAGS} -m32)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also fix a bug where the flag '-m32' was not added when building in 32 bit mode.

Is this right?
But 32 bit mode has been tested in our CI and it showed no errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On my stystem simply replacing the FetchContent function breaks the linking of uvwasi with walrus with a linker error:

/usr/bin/ld: _deps/libuv-build/CMakeFiles/uv.dir/src/fs-poll.c.o: file class ELFCLASS32 incompatible with ELFCLASS64
/usr/bin/ld: final link failed: file in wrong format

And also the most recent x86 CI run of the patch that got merged (commit 7800a31) into the main branch does not include -m32 as an option in it's cflags

- summary of build options:
    Install prefix:  /usr/local
    Target system:   Linux
    Compiler:
      C compiler:    /usr/bin/cc (GNU)
      CFLAGS:         

-- summary of uvwasi build options:

    Install prefix:  /usr/local
    Target system:   Linux
    Compiler:
      C compiler:    /usr/bin/cc
      CFLAGS:         

    LibUV libraries: uv_a
    LibUV includes:  /home/runner/work/walrus/walrus/out/linux/x86/_deps/libuv-src/include
    Debug logging:   
    Code coverage:   
    ASAN:            OFF
    Build tests:     OFF

-- Configuring done (3.1s)
-- Generating done (0.0s)
-- Build files have been written to: /home/runner/work/walrus/walrus/out/linux/x86

There are two possible fixes one of which I recently discovered:

  1. Adding -m32 to CFLAGS or CXXFLAGS so it persists through the whole compilation.
  2. Not changing add_compile_options but adding add_link_options( -m32 ) after it, so the option is present when linking.

I believe that the first method has more information, since the flag -m32 will be visible when running cmake, but I am open to either option.

Copy link
Contributor Author

@kulcsaradam kulcsaradam Jul 25, 2024

Choose a reason for hiding this comment

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

Also an idea:
Should we add a CI that compiles 64 bit walrus on 32 bit system? I don't think that there are CIs for that, but please correct me if I am wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK CFLAGS is not explicitly set by add_compile_options.
The second approach, add_link_options( -m32 ) looks simple and better

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add a CI that compiles 64 bit walrus on 32 bit system?

CI already compiles and tests 32bit mode in 64 bit environments.
I don't think its possible and necessary to build 64 bit in 32 bit systems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK CFLAGS is not explicitly set by add_compile_options. The second approach, add_link_options( -m32 ) looks simple and better

In that case I will change the patch to use that option.

FetchContent_Populate is deprecated, updated the cmake file to use
FetchContent_MakeAvailable.
Also updated minimum cmake version for FetchContent_MakeAvailable
compatibility.
Also fix a bug where the flag '-m32' was not added when building in 32
bit mode.

Signed-off-by: Ádám László Kulcsár <[email protected]>
Copy link
Collaborator

@clover2123 clover2123 left a comment

Choose a reason for hiding this comment

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

LGTM

@clover2123 clover2123 merged commit cde8b86 into Samsung:main Jul 26, 2024
14 checks passed
@kulcsaradam kulcsaradam deleted the cmake_update branch July 26, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants