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

Explicitly include limits.h instead of transitively assuming it #55

Merged
merged 1 commit into from
Jan 2, 2025

Conversation

alerque
Copy link
Contributor

@alerque alerque commented Jan 2, 2025

Recent upstream releases of LuaJIT have broken building this Lua Rock entirely. PUC Lua and LuaJIT builds up to a certain point included this header in lua.h, which meant that it was always around as a transitive dependency. Recent upstream LuaJIT builds no longer include the header since they aren't using it directly any more, but that means things that include lua.h don't get it for free any more. It should never have been assumed anyway, but that's been the status quo for a while.

This fixes build errors when using LuaJIT headers instead of PUC Lua:

lutf8lib.c: In function ‘Lutf8_codepoint’:
lutf8lib.c:398:22: error: ‘INT_MAX’ undeclared (first use in this function)
  398 |   if (pose - posi >= INT_MAX)  /* (lua_Integer -> int) overflow? */
      |                      ^~~~~~~
lutf8lib.c:12:1: note: ‘INT_MAX’ is defined in header ‘<limits.h>’; did you forget to ‘#include <limits.h>’?
   11 | #include "unidata.h"
  +++ |+#include <limits.h>
   12 |
lutf8lib.c:398:22: note: each undeclared identifier is reported only once for each function it appears in
  398 |   if (pose - posi >= INT_MAX)  /* (lua_Integer -> int) overflow? */
      |                      ^~~~~~~

This is because INT_MAX is defined appropriately for a given platfrom in in limits.h.

Recent upstream releases of LuaJIT have broken building this Lua Rock
entirely. PUC Lua and LuaJIT builds up to a certain point included this
header in lua.h, which meant that it was always around as a transitive
dependency. Recent upstream LuaJIT builds no longer include the header
since they aren't using it directly any more, but that means things that
include lua.h don't get it for free any more. It should never have been
assumed anyway, but that's been the status quo for a while.

This fixes build errors when using LuaJIT headers instead of PUC Lua:

```
lutf8lib.c: In function ‘Lutf8_codepoint’:
lutf8lib.c:398:22: error: ‘INT_MAX’ undeclared (first use in this function)
  398 |   if (pose - posi >= INT_MAX)  /* (lua_Integer -> int) overflow? */
      |                      ^~~~~~~
lutf8lib.c:12:1: note: ‘INT_MAX’ is defined in header ‘<limits.h>’; did you forget to ‘#include <limits.h>’?
   11 | #include "unidata.h"
  +++ |+#include <limits.h>
   12 |
lutf8lib.c:398:22: note: each undeclared identifier is reported only once for each function it appears in
  398 |   if (pose - posi >= INT_MAX)  /* (lua_Integer -> int) overflow? */
      |                      ^~~~~~~
```

This is because `INT_MAX` is defined appropriately for a given platfrom
in in limits.h.
Copy link

sonarqubecloud bot commented Jan 2, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@alerque
Copy link
Contributor Author

alerque commented Jan 2, 2025

Here is an example of a CI build that fails because of this issue. Here is a Homebrew formula where I had to patch this to get a formerly working formula to build again.

@alerque
Copy link
Contributor Author

alerque commented Jan 2, 2025

The QA analysis above is returning results from code I didn't touch. I don't think it's taking the PR diff into account.

@starwing starwing merged commit 1bb70d4 into starwing:master Jan 2, 2025
1 of 2 checks passed
@alerque alerque deleted the luajit-headers branch January 2, 2025 13:18
@starwing
Copy link
Owner

starwing commented Jan 2, 2025

Thanks for contributing!

@alerque
Copy link
Contributor Author

alerque commented Jan 2, 2025

Thanks for the quick merge! Will there be a release soon so I can start removing workarounds (and stop wasting my time creating new ones!)? This isn't just a case of wanting new things (where possibly using a Git HEAD dependency can make sense) but lots of existing things using stable tagged releases that have worked for years are suddenly breaking.

@starwing
Copy link
Owner

starwing commented Jan 3, 2025

@alerque done

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