Clean up the addition of extra directories and/or files #125
+34
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The rsync'ing of ~/git/extra-scripts was hacked in before run_qemu was becoming a wider distributed tool. As such it is undocumented and could potentially cause odd or unexpected behavior. The addition of the user-extras provides the same functionality with more user control. Including being explicit for the users benefit. Remove extra-scripts rsync'ing. Signed-off-by: Ira Weiny <[email protected]>
Depending on the use case run_qemu may need additional user space software. ndctl was specifically included and built within the root image due to the initial run_qemu use case to test pmem/dax/cxl. One option would be to hard code the addition of each user space source tree similar to ndctl. This is not desirable for a few reasons. 1) Additional source trees will start to bloat the root image. 2) ndctl was added with very specific directory locations. Some users have worked around this with symlinks but it is inflexible. 3) making source trees optional bloats the parameter space for run_qemu 4) building each user package is very specific to each package A better alternative is to allow the user to specify additional source trees; in different locations. This allows different kernel/user space source combinations in whatever combination the user requires at a specific time. Add the extra-dirs option. The extra-dirs option specifies a file containing a list of paths to user source trees. Each of these source trees is installed in the /root home directory of the guest image. Building of source trees is left to the user. Signed-off-by: Ira Weiny <[email protected]>
marc-hb
reviewed
Mar 10, 2025
| fail "$extra_dirs not found" | ||
| fi | ||
| echo "Installing extra directories and files from: $extra_dirs" | ||
| for d in `cat "$extra_dirs"`; do |
There was a problem hiding this comment.
https://www.shellcheck.net/wiki/SC2006
shellcheck -x run_qemu.sh
Don't forget the -x otherwise there is a ton of false positives.
| { | ||
| local p=$1 | ||
|
|
||
| p=$(eval echo $p) |
| @@ -144,6 +144,21 @@ distro_vars="${script_dir}/${_distro}_vars.sh" | |||
|
|
|||
| pushd "$_arg_working_dir" > /dev/null || fail "couldn't cd to $_arg_working_dir" | |||
|
|
|||
| # save the working directory | |||
| working_dir=$(pwd) | |||
There was a problem hiding this comment.
Why save it again? It's already in "$_arg_working_dir"
| @@ -34,6 +34,7 @@ exit 11 #)Created by argbash-init v2.9.0 | |||
| # ARG_OPTIONAL_BOOLEAN([debug], [v], [Debug script problems (enables set -x)], ) | |||
| # ARG_OPTIONAL_BOOLEAN([ndctl-build], , [Enable ndctl build in root image], [on]) | |||
| # ARG_OPTIONAL_BOOLEAN([kern-selftests], , [Enable kernel selftest build in root image (Warning: This option can take a long time and requires many support packages on the host; including some 32 bit)], [off]) | |||
| # ARG_OPTIONAL_SINGLE([extra-dirs], , [Specify a file with a list of directories (or files) to be copied to the /root directory within the image.], []) | |||
There was a problem hiding this comment.
You need to say that:
- Relative directories inside that file are relative to --working-dir (which defaults to
.) - symbolic links are resolved (NOT recursively like rsync -L does, but that's impossible to summarize clearly and fairly intuitive so don't say "not recursively")
| if [[ $_arg_ndctl_build == "on" ]]; then | ||
| if [ -n "$ndctl" ]; then | ||
| rsync "${rsync_opts[@]}" "$ndctl/" mkosi.extra/root/ndctl | ||
| prepare_ndctl_build # create mkosi.postinst which compiles | ||
| fi | ||
| fi | ||
|
|
||
| if [[ "$_arg_extra_dirs" ]]; then |
There was a problem hiding this comment.
Suggested change
| if [[ "$_arg_extra_dirs" ]]; then | |
| if [[ -n "$_arg_extra_dirs" ]]; then |
| if [[ $_arg_ndctl_build == "on" ]]; then | ||
| if [ -n "$ndctl" ]; then | ||
| rsync "${rsync_opts[@]}" "$ndctl/" mkosi.extra/root/ndctl | ||
| prepare_ndctl_build # create mkosi.postinst which compiles | ||
| fi | ||
| fi | ||
|
|
||
| if [[ "$_arg_extra_dirs" ]]; then | ||
| extra_dirs=$(make_canonical_path $_arg_extra_dirs) |
There was a problem hiding this comment.
I think make_canonical_path should not be used here, otherwise it's incompatible with shell completion?
| fail "$extra_dirs not found" | ||
| fi | ||
| echo "Installing extra directories and files from: $extra_dirs" | ||
| for d in `cat "$extra_dirs"`; do |
There was a problem hiding this comment.
Suggested change
| for d in `cat "$extra_dirs"`; do | |
| cat "$extra_dirs" | while read d; do |
read is guaranteed to read line by line whereas for ... in ... splits on any whitespace.
| if [[ -e $d ]]; then | ||
| rsync -a "$d" mkosi.extra/root/ | ||
| else | ||
| fail "$d not found" |
There was a problem hiding this comment.
Suggested change
| fail "$d not found" | |
| fail "extra-dir directory %s not found" "$d" |
|
BTW, this looks useful and a very welcome cleanup but... I will probably not use it :-) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The ability to add random files and directories to the root image had previously been hacked in via a secret 'special' directory location. This functionality is nice but lacking for a couple of reasons.
Remove the existing hidden feature and add --extra-dirs as a parameter which can specify a list of directories (or files) to be added to /root within the image.
This functionality was discussed in #124
This does not remove or clean up the rsync's but should make one of it's uses much more clear.
To ensure explicit usage for the user no default is provided. But directories will not be removed without a -r wipe.