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.

Sign up for GitHub

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

Jump to bottom

compilation fail on linux aarch64 ppc64le and s390x because of -Werror=type-limits #249

Open
ibotty opened this issue Mar 6, 2024 · 8 comments
Open

compilation fail on linux aarch64 ppc64le and s390x because of -Werror=type-limits #249

ibotty opened this issue Mar 6, 2024 · 8 comments

Comments

@ibotty
Copy link

ibotty commented Mar 6, 2024

See this example log

/usr/bin/g++ -Dloguru_EXPORTS -I/builddir/build/BUILD/loguru-4adaa185883e3c04da25913579c451d3c32cfac1 -O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  -mbranch-protection=standard -fasynchronous-unwind-tables -fstack-clash-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -DNDEBUG -fPIC -Wall -Wextra -Werror -pedantic -MD -MT CMakeFiles/loguru.dir/loguru.cpp.o -MF CMakeFiles/loguru.dir/loguru.cpp.o.d -o CMakeFiles/loguru.dir/loguru.cpp.o -c /builddir/build/BUILD/loguru-4adaa185883e3c04da25913579c451d3c32cfac1/loguru.cpp
/builddir/build/BUILD/loguru-4adaa185883e3c04da25913579c451d3c32cfac1/loguru.cpp: In function ‘void loguru::escape(std::string&, const std::string&)’:
/builddir/build/BUILD/loguru-4adaa185883e3c04da25913579c451d3c32cfac1/loguru.cpp:571:36: error: comparison is always true due to limited range of data type [-Werror=type-limits]
  571 |                         else if (0 <= c && c < 0x20) { // ASCI control character:
      |                                  ~~^~~~
/builddir/build/BUILD/loguru-4adaa185883e3c04da25913579c451d3c32cfac1/loguru.cpp: In function ‘loguru::Text loguru::ec_to_text(char)’:
/builddir/build/BUILD/loguru-4adaa185883e3c04da25913579c451d3c32cfac1/loguru.cpp:1845:28: error: comparison is always true due to limited range of data type [-Werror=type-limits]
 1845 |                 else if (0 <= c && c < 0x20) {
      |                          ~~^~~~
cc1plus: all warnings being treated as errors
@virtuosonic
Copy link

i got the same error under GCC arm-linux-gnueabihf

@virtuosonic
Copy link

https://github.com/virtuosonic/loguru/ fixes this

@musicinmybrain
Copy link

In the new loguru package for Fedora, I’m dealing with this by just removing -Werror, which I think is too strict for downstream packaging in general:

sed -r -i 's/;?-Werror\b//' CMakeLists.txt test/CMakeLists.txt

It’s pretty clear that the warning, while technically correct, is not indicating a real problem here.

The patch virtuosonic@e1ffdc4 from the fork linked above does look, at a glance, like a correct way to avoid the warning without changing the code’s behavior across architectures and compilers.

@virtuosonic
Copy link

virtuosonic commented May 28, 2024 •
edited
Loading

The patch virtuosonic@e1ffdc4 from the fork linked above does look, at a glance, like a correct way to avoid the warning without changing the code’s behavior across architectures and compilers.

Actually there no risk if the variable c in the loops is signed or unsigned because it isn't modified, my next step would be to make it const, to make this clear to the compiler.

@skaravos
Copy link
Contributor

In the new loguru package for Fedora, I’m dealing with this by just removing -Werror, which I think is too strict for downstream packaging in general:

You're probably right, when I originally wrote the CMakeLists.txt file I just added a fairly aggressive block of warning flags to ensure I wasn't breaking anything super obvious. I definitely didn't consider the impact that -Werror would have on package maintainers.
Would it be helpful to add a LOGURU_WARNINGS_AS_ERRORS cache variable, or perhaps a LOGURU_DEVELOPER_MODE cache variable that could be used to conditionally add the warning flags?

In either case, the default out-of-the-box CMakeLists.txt should definitely not have -Werror.

@musicinmybrain
Copy link

In the new loguru package for Fedora, I’m dealing with this by just removing -Werror, which I think is too strict for downstream packaging in general:

[…] Would it be helpful to add a LOGURU_WARNINGS_AS_ERRORS cache variable, or perhaps a LOGURU_DEVELOPER_MODE cache variable that could be used to conditionally add the warning flags?

In either case, the default out-of-the-box CMakeLists.txt should definitely not have -Werror.

Sure, a CMake option that defaults to disabling -Werror sounds like a reasonable idea. I do definitely support using -Werror in upstream development and CI.

@drupol
Copy link

drupol commented Nov 3, 2024 •
edited
Loading

Hello,

I'm trying to package Docling in Nix (NixOS/nixpkgs#353183) and loguru is a dependency of docling-parse. I am was unable to build loguru on Darwin until I added 2 patches from https://github.com/virtuosonic/loguru/ that fixes the issues gracefully.

skaravos added a commit to skaravos/loguru that referenced this issue Nov 4, 2024
@skaravos
Add cmake option LOGURU_ENABLE_COMPILER_WARNINGS
19a9638 
Changes:
- Provides a workaround for
  [emilk#249](emilk#249)
- Adds an option that can be used to disable compile warnings fork
  the loguru library. This could be useful for users who just want to
  build the library and dont care about the warnings.
  (or they just aren't in a position to fix them anyways).

Notes:
- The default behaviour remains the same: compiler-warnings are enabled
  by default when the project is built as a top-level project, and
  disabled by default when the project is included as a sub-project with
  either add_subdirectory() or FetchContent()
skaravos added a commit to skaravos/loguru that referenced this issue Nov 4, 2024
@skaravos
Add loguru cmake option LOGURU_WARNINGS_AS_ERRORS
57ba757 
Changes:
- Provides a workaround for [emilk#249](emilk#249)
- Removed `-Werror` from the default compiler warnings for GCC.
- Added an option that can be used to opt-in to warnings-as-errors for
  all three supported compilers (gcc, clang, msvc) if desired.
skaravos added a commit to skaravos/loguru that referenced this issue Nov 4, 2024
@skaravos
Add cmake option LOGURU_ENABLE_COMPILER_WARNINGS
808c173 
Changes:
- Provides a workaround for
  [emilk#249](emilk#249)
- Adds an option that can be used to disable compile warnings for
  the loguru library. This could be useful for users who just want to
  build the library and dont care about the warnings.
  (or they just aren't in a position to fix them anyways).

Notes:
- The default behaviour remains the same: compiler-warnings are enabled
  by default when the project is built as a top-level project, and
  disabled by default when the project is included as a sub-project with
  either add_subdirectory() or FetchContent()
skaravos added a commit to skaravos/loguru that referenced this issue Nov 4, 2024
@skaravos
Add loguru cmake option LOGURU_WARNINGS_AS_ERRORS
0947bd7 
Changes:
- Provides a workaround for [emilk#249](emilk#249)
- Removed `-Werror` from the default compiler warnings for GCC.
- Added an option that can be used to opt-in to warnings-as-errors for
  all three supported compilers (gcc, clang, msvc) if desired.
skaravos added a commit to skaravos/loguru that referenced this issue Nov 4, 2024
@skaravos
Add cmake option LOGURU_ENABLE_COMPILER_WARNINGS
fc2a26f 
Changes:
- Provides a workaround for
  [emilk#249](emilk#249)
- Adds an option that can be used to disable compile warnings for
  the loguru library. This could be useful for users who just want to
  build the library and dont care about the warnings.
  (or they just aren't in a position to fix them anyways).

Notes:
- The default behaviour remains the same: compiler-warnings are enabled
  by default when the project is built as a top-level project, and
  disabled by default when the project is included as a sub-project with
  either add_subdirectory() or FetchContent()
skaravos added a commit to skaravos/loguru that referenced this issue Nov 4, 2024
@skaravos
Add loguru cmake option LOGURU_WARNINGS_AS_ERRORS
a784453 
Changes:
- Provides a workaround for [emilk#249](emilk#249)
- Removed `-Werror` from the default compiler warnings for GCC.
- Added an option that can be used to opt-in to warnings-as-errors for
  all three supported compilers (gcc, clang, msvc) if desired.
@drupol
Copy link

drupol commented Nov 4, 2024 •
edited
Loading

Fixed in nix by adding 2 patches, find the derivation at https://github.com/NixOS/nixpkgs/blob/f6ecda6ad047ca780874895cb98d8643c6890f6f/pkgs/by-name/lo/loguru/package.nix

This builds successfully on Linux and Darwin (macos) on aarch64 and x86_64.

@emilk would it be possible to cherry pick these patches in the official version?

@skaravos skaravos mentioned this issue Nov 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@drupol @ibotty @musicinmybrain @virtuosonic @skaravos