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 failures on macOS #107

Closed
bkirz opened this issue Apr 28, 2023 · 8 comments
Closed

Build failures on macOS #107

bkirz opened this issue Apr 28, 2023 · 8 comments

Comments

@bkirz
Copy link
Contributor

bkirz commented Apr 28, 2023

I'm running into errors building some dependencies on macOS 13.3.1. Here's a full build log if I run ./Utils/build-release-macos.sh: https://gist.github.com/bkirz/02660a7e26d0dcfbfe2128c7189fa84a

Specifically, I'm seeing two classes of errors:

  • In mbedtls, I get the failure /Users/bkirz/dev/itgmania/extern/mbedtls/library/bignum.c:1395:29: error: variable 't' set but not used [-Werror,-Wunused-but-set-variable] mbedtls_mpi_uint c = 0, t = 0; ^
  • In zlib, I get multiple failures of the form error: call to undeclared function 'lseek'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration].

I did some googling but didn't see sny obvious solutions for either problem. Would bumping the version of either library help? Is that even feasible? I'm not sure how up-to-date SM5's dependencies are kept.

Test Environment

  • 2021 MacBook Pro with M1 Pro
  • macOS Ventura 13.3.1
  • Tested with bcea05d and 1b24361
@bkirz
Copy link
Contributor Author

bkirz commented Apr 28, 2023

Looks like the zlib failure shows up in an unrelated project: bazelbuild/bazel#17956

There are some proposed fixes in the issue's comments, including a three-line patch and changing the flags used to build zlib, not sure of the best approach here.

The mbedtls issue was raised here: Mbed-TLS/mbedtls#7166 and seems to have a fix applied?

My hunch is that some macOS version bump also bumped clang's version and caused some previously benign issues to raise warnings.

@bkirz
Copy link
Contributor Author

bkirz commented May 8, 2023

I forgot to note that on the beta branch, tomcrypt also fails to build with similar errors to zlib. 😞

I've been able to fix the mbedtls errors by pointing the submodule to version 3.4.0, although zlib and tomcrypt don't appear to have fixes pushed to a published version. If it's possible to apply a patch to zlib, we'd be able to copy this: Blosc/c-blosc@ac72c2c. I don't see a way to apply patches that's built into git submodules, but perhaps there's a way to do it using stepmania's build system?

@bkirz
Copy link
Contributor Author

bkirz commented May 8, 2023

Another idea that I've seen mentioned is changing clang's build args to ignore this warning. I'm new to C++ and CMake but can dig into the feasibility of doing that since it seems preferable over the patch approach. While we could make that work for zlib, I'm unaware of a publicly available patch for tomcrypt and have no idea where to start with rolling my own.

@bkirz
Copy link
Contributor Author

bkirz commented May 20, 2023

A few updates on this:

  • These errors still appear after upgrading to Ventura 13.4.
  • Given that the errors are all related to "ISO C99 and later", I tried updating the extern/CMakeProject-*.cmake files for tomcrypt and zlib with lines like these:
    set_property(TARGET "zlib" PROPERTY CXX_STANDARD 98)
    set_property(TARGET "zlib" PROPERTY CXX_STANDARD_REQUIRED ON)
    I don't think those are doing what I want, since they don't change the error messaging.

@geefr
Copy link
Contributor

geefr commented May 20, 2023

@bkirz I'm not sure if downgrading the requirement would be the correct fix / don't have time to dig into this myself, but:

Try C_STANDARD instead of CXX_STANDARD and it might do it - C and C++ standards are different things, and zlib is a mostly C library.

For the warnings - It's entirely valid to not treat warnings as errors for 3rd party dependencies. That might just be a top level '-Werror=all' or similar for the itgmania code, that's then inherited through the CMake file hierarchy.
Building with make V=1 VERBOSE=1 should log the full build commands, if you need to see what '-Werror' flags are being passed to the eventual compiler command.

And for zlib - I'd guess it's a missing include or something, maybe fixed in a newer zlib? You're on the right track though as the two fixes are to update zlib itself (and risk breaking other platforms), or tweak the compile flags to be happy.

@bkirz
Copy link
Contributor Author

bkirz commented May 21, 2023

Thanks for the tips @geefr! I tried swapping to C_STANDARD and got a new error:

CMake Error in extern/CMakeLists.txt:
  Target "zlib" requires the language dialect "C98" (with compiler
  extensions).  But the current compiler "AppleClang" does not support this,
  or CMake does not know the flags to enable it.

Not what I was hoping for, but at least we can rule out that option now. Seems changing the args is the next best option since updates to zlib and tomcrypt don't yet exist that fix these issues. I'm not sure how to set arbitrary warning config with CMake and will dig into that next.

I'm running into a weird issue with calling make directly:

CMake Error at CMake/SetupFfmpeg.cmake:45 (message):
  Unsupported macOS architecture: , set CMAKE_OSX_ARCHITECTURES to either
  arm64 or x86_64
Call Stack (most recent call first):
  StepmaniaCore.cmake:215 (include)
  CMakeLists.txt:5 (include)

Strangely, this persists if I set CMAKE_OSX_ARCHITECTURES as an env arg. I suspect I'll have to sit down and Actually Learn how to use CMake to get around this.

bkirz added a commit to bkirz/itgmania that referenced this issue May 22, 2023
bkirz added a commit to bkirz/itgmania that referenced this issue May 22, 2023
@bkirz
Copy link
Contributor Author

bkirz commented May 22, 2023

Turns out I just needed to swap out the 98 for 90 to get this build passing. Opened #110 to fix the issue.

teejusb pushed a commit that referenced this issue May 22, 2023
@bkirz bkirz closed this as completed Jun 19, 2023
@aeubanks
Copy link
Contributor

aeubanks commented Oct 16, 2023

I'm seeing this on Linux too with a recent clang, the fix (edit: *workaround) probably shouldn't be limited to mac

aeubanks added a commit to aeubanks/itgmania that referenced this issue Oct 24, 2023
I'm seeing the same warning building with clang on Linux.
teejusb pushed a commit that referenced this issue Oct 24, 2023
I'm seeing the same warning building with clang on Linux.
mjvotaw added a commit to mjvotaw/itgmania that referenced this issue Feb 13, 2024
* beta:
  When sorting course songs by GRADEBEST or GRADEWORST, check if GAMESTATE has a master player before calling SongUtil::SortSongPointerArrayByGrades().
  Added missing GRADEBEST and GRADEWORST cases for CourseWriterCRS
  Automatically create Songs folder on Memory Cards
  Bump actions/checkout from 3 to 4
  pref to bass simplify lighting
  MergeFromOtherHSL() now 'Removes All But One Of Each Name' per preference
  nudge enchantment noteskin's first two frames 1px
  Fix groove radar value calculation
  Make tomcrypt workaround for itgmania#107 apply everywhere

# Conflicts:
#	src/Course.cpp
#	src/CourseWriterCRS.cpp
aeubanks added a commit to aeubanks/itgmania that referenced this issue Apr 20, 2024
teejusb pushed a commit that referenced this issue Apr 21, 2024
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

No branches or pull requests

3 participants