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

Make lua a git submodule #6549

Merged
merged 7 commits into from Mar 9, 2022
Merged

Conversation

Pentarctagon
Copy link
Member

This PR removes the in-tree lua code to instead be a git submodule. This is done by:

  • Reverting the static_cast change to the lua source code - wrap the direct lua/ includes in one of the headers in the nowarn/ directory, which contains the necessary changes (mostly #pragmas) to ignore the old style cast warnings. This is kinda hacky, but I wasn't able to find a better way to suppress warnings from external header files.
  • Not renaming the .c files to .cpp - instead specify those files to be compiled as C++ via the command line (-x c++ for gcc/clang, /TP for MSVC).

The main benefit being that we'd be using the unmodified lua source code, which as a submodule means that updating to a new version of lua can be done just by going into the submodule's directory, doing a git checkout of the next tagged version, and pushing that update to Wesnoth's repo.

@Pentarctagon
Copy link
Member Author

@Vultraz FYI as well.

@github-actions github-actions bot added AI Issues with the AI engine, including micro AIs. Building Build-time issues. CMake Issues involving the CMake build system. Lua API Issues with the Lua engine and API. macOS OS-specific issues that apply to Apple macOS. SCons Issues involving the SCons build system. labels Mar 3, 2022
@Wedge009 Wedge009 removed macOS OS-specific issues that apply to Apple macOS. AI Issues with the AI engine, including micro AIs. labels Mar 3, 2022
Copy link
Member

@CelticMinstrel CelticMinstrel left a comment

Choose a reason for hiding this comment

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

I don't really like the naming of the "nowarn" directory. Since the "lua" directory no longer exists, why not just rename "nowarn" to "lua"?

Do the two files that include luaconf.h actually need to? I think that file should be indirectly included by any other file, shouldn't it? I have the same question about llimits.h.

I can see the MacOS CI passed but I'm not sure why – what exactly makes it compile those files as C++? When I was looking at the diff, I thought it would break the build since there's no file-specific flags on those files (as far as I know).

On the whole I'm fairly ambivalent about this. It does make it harder for newcomers to grab the code though.

src/nowarn/lauxlib.h Outdated Show resolved Hide resolved
@github-actions github-actions bot added AI Issues with the AI engine, including micro AIs. macOS OS-specific issues that apply to Apple macOS. labels Mar 4, 2022
@Pentarctagon
Copy link
Member Author

Pentarctagon commented Mar 4, 2022

I don't really like the naming of the "nowarn" directory. Since the "lua" directory no longer exists, why not just rename "nowarn" to "lua"?

Sure.

Do the two files that include luaconf.h actually need to? I think that file should be indirectly included by any other file, shouldn't it? I have the same question about llimits.h.

I fixed one instance of unnecessary includes since I happened to notice it, but I didn't look into that any further.

I can see the MacOS CI passed but I'm not sure why – what exactly makes it compile those files as C++? When I was looking at the diff, I thought it would break the build since there's no file-specific flags on those files (as far as I know).

It looks like xcode is smart enough to use -x c++ for the lua files, though I couldn't tell you why. Most likely though there's a setting somewhere for the lua files to force them to be built as C++.

On the whole I'm fairly ambivalent about this. It does make it harder for newcomers to grab the code though.

Hmm... yeah, that's true. Mariadbpp is also a submodule, though I suppose a lot fewer people would be likely to run into it.

Edit-
Though the only difference would be either running git submodule update --init --recursive separately after the clone, or cloning with the --recurse-submodules option.

@Pentarctagon
Copy link
Member Author

Updated to rename the folder to lua, and cleaned up the includes.

@CelticMinstrel
Copy link
Member

Though the only difference would be either running git submodule update --init --recursive separately after the clone, or cloning with the --recurse-submodules option.

Yes. It's still an extra step, however.

@Pentarctagon
Copy link
Member Author

I can add checks to scons/cmake to print an error telling what command they need to run, though that wouldn't help macOS users. There's also a post-checkout hook which also runs on clone, however client-side hooks aren't downloaded during said cloning, so that'd still be an extra step to setup somewhere.

I don't think it's a big issue, but it being a submodule is also standalone from the rest of the changes, so the main question if that needs to be reverted is just where to put the headers suppressing the -Wold-style-cast warnings (since they of course can't exist in the lua/ directory anymore at that point.

@Vultraz
Copy link
Member

Vultraz commented Mar 8, 2022

Damn, I just noticed how many lua paths were able to be cleaned up p_p I like the current state this is in.

@Pentarctagon Pentarctagon force-pushed the lua-suppress branch 2 times, most recently from 520b67d to a95a7d9 Compare March 9, 2022 03:46
Being .cpp isn't required to have them be compiled as C++.
No changes required to the lua source anymore.
Also cleanup the includes.
@Pentarctagon Pentarctagon merged commit d32cfb8 into wesnoth:master Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AI Issues with the AI engine, including micro AIs. Building Build-time issues. CMake Issues involving the CMake build system. Lua API Issues with the Lua engine and API. macOS OS-specific issues that apply to Apple macOS. SCons Issues involving the SCons build system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants