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
Fix: [CMake] Check nforenum and grfcodec return value #9085
Conversation
Honestly, I am not sure this is the correct solution. If you manually supply stuff, it should just try to execute that, and not evaluate if it is correct. For me it is such a slippery slope .. should we also check if it is really grfcodec, and not some other binary? :D Where does this end ;) But aren't you running into a completely different problem, one I am having currently: if grfcodec runtime fails, CMake continues to build OpenTTD. Which is very .. euh .. annoying. Example: change the sprite-count in Maybe if we solve that issue, the reason you created this issue is also resolved? tldr, for me the keyword in your PR is |
execute_process(COMMAND ${GRFCODEC_EXECUTABLE} -n -s -e -p1 ${GRF_SOURCE_FOLDER_NAME}.grf) | ||
execute_process(COMMAND ${NFORENUM_EXECUTABLE} -s sprites/${GRF_SOURCE_FOLDER_NAME}.nfo RESULT_VARIABLE RESULT) | ||
if(RESULT) | ||
if(NOT RESULT MATCHES "^[0-9]*$") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this regex capture? Maybe a comment of some kind might help future-us, but mainly I am just curious :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RESULT
can be "an integer return code from the last child or a string describing an error condition" as stated in CMake documentation. So if it's not an integer it didn't run at all and there's probably an issue with the executable itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smart :D
Motivation / Problem
When
GRFCODEC_EXECUTABLE
andNFORENUM_EXECUTABLE
are manually specified, they are considered valid andGRFCODEC_FOUND
is set, but the variables could contain errors and execution silently fail later.Description
Always try to runGRFCODEC_EXECUTABLE
andNFORENUM_EXECUTABLE
and discard invalid values.Check return value when running nforenum and grfcodec, and force the build to fail if needed.
I also fixed a dependency check error (modifying a .nfo would not trigger a grf rebuild)
Limitations
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.