-
Notifications
You must be signed in to change notification settings - Fork 90
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
improve CPIO creation #151
base: master
Are you sure you want to change the base?
Conversation
axel-h
commented
Jul 7, 2022
•
edited
Loading
edited
- reduce redundancy by using some helper variables
- use verbose cpio flags
- Improve maintainability for CPIO creation
- allow passing a directory also in the CPIO archive name, so root binary folder pollution can be avoided
a62431a
to
e7b4de2
Compare
e7b4de2
to
7967e5a
Compare
7967e5a
to
6a828bf
Compare
@@ -36,11 +36,11 @@ endfunction() | |||
# into another target | |||
function(MakeCPIO output_name input_files) | |||
cmake_parse_arguments(PARSE_ARGV 2 MAKE_CPIO "" "CPIO_SYMBOL" "DEPENDS") | |||
if(NOT "${MAKE_CPIO_UNPARSED_ARGUMENTS}" STREQUAL "") |
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.
This pattern is repeated a lot throughout the CMake files. I recall it's partially because if (${MAKE_CPIO_UNPARSED_ARGUMENTS})
unexpectedly evaluates to false for almost all string values as CMake's described behavior (https://cmake.org/cmake/help/latest/command/if.html).
Using if(MAKE_CPIO_UNPARSED_ARGUMENTS)
is shorter and you know that this tests if the variable is defined and defined to non-false values. But it would be easy to misread something like if(${MAKE_CPIO_UNPARSED_ARGUMENTS}
or if("${MAKE_CPIO_UNPARSED_ARGUMENTS}"
) as testing for truth if you aren't more familiar with the if() gotchas . In these cases, if the variable contains a string it will always evaluate to false unless the contents match the truthy string values (eg ON, TRUE, etc). So while "${MAKE_CPIO_UNPARSED_ARGUMENTS}" STREQUAL "")
is more verbose, it's probably a good pattern to leave around for explicitly testing for a non-empty string as it doesn't rely on understanding any if() testing gotchas.
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.
I agree with all points. This is the outcame from digging into CMake usage and a long term plan to use the language as it seems intended nowadays. I can move this to a separate PR to make the intention more clear. There are more places where this could be changed to clarify what checks are non-trivial.
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.
I have remove the commit
Fixed a few typos. |
Rebased. Can we merge this now, do the other PRs that use this get unblocked? |
c73732d
to
506409a
Compare
9bf8465
to
b267344
Compare
1230338
to
c5647e6
Compare
06a5c1f
to
a83207b
Compare
0b9437f
to
ff1e8c9
Compare
ff1e8c9
to
2dfa617
Compare
rebased and improved the absolute/relative path handling a bit more. |
When I originally approved this, it was a much smaller change: e5981ae, so my approval doesn't apply to the current version of the PR. |
# Re-run CMake configuration in case 'cpio_archive_creator' or | ||
# 'cpio_archive_s' is deleted. | ||
set_property( | ||
DIRECTORY | ||
APPEND | ||
PROPERTY CMAKE_CONFIGURE_DEPENDS "${cpio_archive_creator}" "${cpio_archive_s}" | ||
) |
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.
I don't consider that this property is needed unless CMake implicitly adds it for all files it generates in the build directory already. Trying to detect and recover the build directory becoming corrupted isn't the responsibility of the build scripts.
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.
It's a convenience feature I found quite useful when experimenting with the archive creation. In general, when debugging CMake things, I often corrupt the build folder or delete files on purpose to force partial regeneration. And my expectation from proper dependency management is, that everything in the build folder should be re-generated automatically.
2dfa617
to
aa87728
Compare
The older approach did not work in some corner cases. |
Define helper variables to reduce redundancy Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
- improve maintainability with explicit helper files - sanitize intermediate file names - improve comments - support using a directory in output name also Signed-off-by: Axel Heider <axel.heider@hensoldt.net>