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

improve CPIO creation #151

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

axel-h
Copy link
Member

@axel-h axel-h commented Jul 7, 2022

  • 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

@@ -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 "")
Copy link
Member

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.

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 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.

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 have remove the commit

@axel-h
Copy link
Member Author

axel-h commented Mar 21, 2023

Fixed a few typos.

kent-mcleod
kent-mcleod previously approved these changes Mar 21, 2023
@axel-h
Copy link
Member Author

axel-h commented Mar 28, 2023

Rebased. Can we merge this now, do the other PRs that use this get unblocked?

@axel-h
Copy link
Member Author

axel-h commented Feb 23, 2024

rebased and improved the absolute/relative path handling a bit more.

@kent-mcleod
Copy link
Member

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.

@kent-mcleod kent-mcleod self-requested a review February 24, 2024 21:21
Comment on lines +105 to +112
# 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}"
)
Copy link
Member

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.

Copy link
Member Author

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.

@axel-h
Copy link
Member Author

axel-h commented Feb 25, 2024

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.

The older approach did not work in some corner cases.

Axel Heider added 3 commits July 2, 2024 16:06
Define helper variables to reduce redundancy

Signed-off-by: Axel Heider <[email protected]>
Signed-off-by: Axel Heider <[email protected]>
- 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hw-build enable all sel4test hardware builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants