Skip to content

Conversation

@GraithTiger
Copy link

Related to #515, liblua52.so can be built using the system CFLAGS variable. I noticed when building/modifying the AUR package that running the build script only included -fPIC, and built using only -O2 -Wall -DLUA_COMPAT_ALL -DLUA_USE_LINUX -fPIC. This didn't compile using the debug options set by makepkg.

Adding ${CFLAGS} to MYCFLAGS in the build script allowed the library to compile with those symbols, as well as other security mitigations. SYSCFLAGS is set explicitly in Lua's Makefile, so it's safer to change MYCFLAGS and MYLDFLAGS instead without patching.

@GraithTiger GraithTiger requested a review from shpaass as a code owner August 14, 2025 21:36
@shpaass
Copy link
Owner

shpaass commented Aug 14, 2025

Hello and welcome to Yafc! Thank you for the contribution!
I think @veger would like to take a look at that.

Copy link
Collaborator

@veger veger left a comment

Choose a reason for hiding this comment

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

I deliberately left them out as I wanted to make this 'fool proof' and assume that someone requiring debug symbols would know how to modify the script and add the according variables en enable them.

As an middle ground, we could add some message (echo) telling anyone running the script, that adding custom build flags needs to be done in the script directly.

I am fine by this change, as it is not wrong, but would like the final decision by @shpaass: whether we keep it 'fool proof' or allow for easy tinkering (one could argue that only people who are tinkering will use it, so we need to facilitate them with this PR... 🤔 ).

@shpaass
Copy link
Owner

shpaass commented Aug 20, 2025

What do you guys think about using the flags but thoroughly explaining what they do in the comment above the line that the PR changes? A person may have no idea what CFLAGS and LDFLAGS are, so let's describe their purpose and contents a bit.
This way a person who potentially encounters an error would have more context to decide on the next action.

@veger
Copy link
Collaborator

veger commented Aug 20, 2025

If someone knows how to set CFLAGS and LDFLAGS they do not need the documentation I suppose (as they installed a GCC dev environment in order to use them, they know how to set environmental flags, etc. So I would assume they are developers).
Non-developer people do not need to recompile liblua, let alone recompiling with extra compiler/linker flags...

But since you mentioned documentation: It might be nice(r) to add a sentence that these are supported by the script in the README.md in the Lua folder. So other devs know they can be used. I think that is a more obvious place than inside the script?

@shpaass
Copy link
Owner

shpaass commented Aug 25, 2025

Rebased on fresh master

@shpaass
Copy link
Owner

shpaass commented Aug 25, 2025

Squashed fixups

@shpaass
Copy link
Owner

shpaass commented Aug 25, 2025

Added a changelog note that mentions the rebuild of the linux lib because it was not mentioned in #515.

@shpaass shpaass merged commit 8ccde24 into shpaass:master Aug 25, 2025
1 check passed
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.

4 participants