Skip to content

Conversation

@Desour
Copy link
Member

@Desour Desour commented Oct 8, 2025

  • Adds a list of builtin lua files in cmake. (like in src) (unittest files are not included)
  • Generates a cpp file with relative paths and hashes.
  • Prints warnings if you try to loadfile (or dofile) a path in builtin (with abs path) if either the file is not in the cmake list or if the hash mismatches.
    Fixes Make builtin check for expected engine version #16318.
  • Code cleanup of ScriptApiSecurity::safeLoadFile.
    (And CPCSM files will now have the shebang ignored (but it looks like luajit already does that).)
  • (Will also be useful for SSCSM client builtin (check to prevent cheating) and SSCSM server builtin (to find out which files to send).)
  • Doesn't work for CPCSM.
  • I've decided for warnings and not errors, as I'm worried that otherwise the user might be left without a working main menu (which right now results in a "No future without main menu!" plus abort) and not know what is happening.
  • Side effect: The feature to load code from stdin with dofile() and loadfile() (without args) is removed.
    (ContendDB grep had no true positive results.)

To do

This PR is Ready for Review.

TODO:

  • Idk how to properly escape paths in cmake. I.e. if a " is in a path, it might split the list. (see github review comment below)
  • On macOs there are issues with the file being added to multiple targets. TODO

How to test

  • Start and notice how there are no warnings.
  • Change a builtin file, e.g. after.lua. Observe the warning.
  • Rebuild, and observe that it regenerates the hashes.
  • Comment out a file in CMakeLists.txt. Observe the other warning.
  • /lua dofile()
  • Look into the generated builtin_files.cpp.

Desour added 16 commits October 8, 2025 17:24
TODOs:
* move it somewhere else
* abs path
* mod security off codepath
* exception instead of returning false
* fallback mainmenu
(a bunch of `ls -1`)
(should maybe not use add_custom_command then?)
(Should make shebangs work in CPCSM. (Though it looks like luajit already
ignores shebangs. 🤷))
@Desour Desour added @ Build CMake, build scripts, official builds, compiler and linker errors @ Builtin labels Oct 8, 2025
@Desour Desour changed the title Print warnings if sha26 of builtin files do not match with build Builtin: Print warnings if sha26 of files do not match with build Oct 8, 2025
@Desour Desour marked this pull request as draft October 8, 2025 19:52
@Zughy Zughy added Feature ✨ PRs that add or enhance a feature Roadmap: supported by core dev PR not adhering to the roadmap, yet some core dev decided to take care of it labels Oct 9, 2025
@sfan5 sfan5 changed the title Builtin: Print warnings if sha26 of files do not match with build Builtin: Print warnings if SHA256 of files does not match with build Oct 9, 2025
Desour added 2 commits October 9, 2025 16:35
this should make GenerateBuiltinFilesCpp a common dependency of EngineCommon,
I think, so fix this:

>  The custom command generating
>
>    /Users/runner/work/luanti/luanti/build_xcode/src/builtin_files.cpp
>
>  is attached to multiple targets:
>
>    GenerateBuiltinFilesCpp
>    EngineCommon
>
>  but none of these is a common dependency of the other(s).  This is not
>  allowed by the Xcode "new build system".
@Desour Desour marked this pull request as ready for review October 9, 2025 14:52
@Desour Desour marked this pull request as draft October 9, 2025 15:00
@Desour Desour force-pushed the build_builtin_hash branch from 944417f to f0c5835 Compare October 9, 2025 15:34
@SmallJoker
Copy link
Member

Things I don't like about this concept:

  1. CMakeLists.txt for Lua files seems cumbersome, especially because we're taking the entire builtin dir when packaging.
  2. A 256-bit hash is not needed. Adler-32 or the murmur hash would be A) much faster and B) fit into an integer value.

My approach would be to write a small C++ program that traverses the directory and generates the source file required by the main luanti CMake target.

I hate cmake.
also makes GenerateBuiltinFilesList.cmake simpler
it generates a cpp file, so no src file will include it. hence clang tidy *should* not
need it, right?


# Command for builtin_files.cpp
add_custom_command(
Copy link
Member Author

Choose a reason for hiding this comment

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

For clarity, this is not the same as for GenerateVersion, because GenerateVersion rebuilds every time.
(I hope the comments and DEPENDS make this obvious enough for future readers.)

@Desour
Copy link
Member Author

Desour commented Oct 11, 2025

Things I don't like about this concept:

1. `CMakeLists.txt` for Lua files seems cumbersome, especially because we're taking the entire builtin dir when packaging.

My approach would be to write a small C++ program that traverses the directory and generates the source file required by the main luanti CMake target.

CMake has a glob feature, no need to reimplement it in C++. It was already discussed here: #16553 (comment)

2. A 256-bit hash is not needed. Adler-32 or the murmur hash would be A) much faster and B) fit into an integer value.

Sounds like premature optimization to me.
Also, as said I'd like to use it for sscsm client builtin in the future, and there we need secure hashes (otherwise you can add a comment to the file to make the hash match (though at that point, rebuilding might be easier (well, but not if someone makes a small program that does the adjustments for you))).

This reverts commit ef6bc53.

the clang tidy ci does need it
-DENABLE_GETTEXT=FALSE \
-DBUILD_SERVER=TRUE
cmake --build build --target GenerateVersion
cmake --build build --target GenerateVersion GenerateBuiltinFilesCpp
Copy link
Member Author

Choose a reason for hiding this comment

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

I think it might also work to pass build/src/builtin_files.cpp here. One should be able to get rid of GenerateBuiltinFilesCpp then. But this seemed cleaner for me.

@Desour Desour marked this pull request as ready for review October 11, 2025 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

@ Build CMake, build scripts, official builds, compiler and linker errors @ Builtin Feature ✨ PRs that add or enhance a feature Roadmap: supported by core dev PR not adhering to the roadmap, yet some core dev decided to take care of it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make builtin check for expected engine version

5 participants