Skip to content

Clean up the addition of extra directories and/or files #125

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Clean up the addition of extra directories and/or files #125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd77e58
run_qemu: Remove extra-scripts hidden option
weiny2 Mar 7, 2025
2d688f3
run_qemu: Add extra-dirs option
weiny2 Mar 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions parser_generator.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.], [])
Copy link
Collaborator

Choose a reason for hiding this comment

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

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")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to keep the help small. But yea it is probably worth noting that any relative paths are to the working directory. I'll update it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also: shell expansion that I mostly miss in the first review. It's really not a small thing.

# ARG_OPTIONAL_INCREMENTAL([quiet], [q], [quieten some output, can be repeated multiple times to quieten even more], )
# ARG_OPTIONAL_BOOLEAN([gdb], , [Wait for gdb to connect for kernel debug (port 10000)], )
# ARG_OPTIONAL_BOOLEAN([gdb-qemu], , [Start qemu with gdb], )
Expand Down
37 changes: 33 additions & 4 deletions run_qemu.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why save it again? It's already in "$_arg_working_dir"

Copy link
Contributor Author

@weiny2 weiny2 Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

pwd guarentees we have that directory's full path after doing the push where ever _arg_working_dir was.

FWIW AFAIK no one uses _arg_working_dir. So it is probably almost untested. But this seemed safe.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yes of course. Then please:

Suggested change
working_dir=$(pwd)
abs_working_dir=$(pwd)

Please also drop or change the comment, it threw me off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok sure!


# make a path canonical to the working dir
make_canonical_path()
{
local p=$1

p=$(eval echo $p)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this line do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This really sent me down a rabit hole which I could not resolve any other way.

If you don't do the eval on the $arg* variable any shell expansion will not happen. So without that eval this:

--extra-dirs=~/foo

Will not resolve to your home dir. I tried for a while to get it to work. For some reason this only happens with the argbash variables and not with other variables defined in the script. Running eval made the shell expansion work as expected. So I went with it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I missed that shell expansion was an intended feature. It's really not a small thing that should be found by chance in line 155 and nowhere else.

Copy link
Member

@stellarhopper stellarhopper Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hm I don't love the eval.. (also is the echo redundant anyway?)

Does something like this work?

paths=($_arg_extra_dirs)

for paths in ${paths[@]}; do
    <stuff>
done

pushd $working_dir > /dev/null
p=$(realpath $p)
popd > /dev/null
echo $p
}

set_valid_mkosi_ver()
{
"$mkosi_bin" --version
Expand Down Expand Up @@ -1125,6 +1140,7 @@ make_rootfs()
process_mkosi_template "$tmpl" > "$dst"
done

# Add user space items from host
rsync -a "${script_dir}"/mkosi/extra/ mkosi.extra/

# misc rootfs setup
Expand All @@ -1148,17 +1164,30 @@ make_rootfs()
if [ -f ~/.vimrc ]; then
rsync "${rsync_opts[@]}" ~/.vim* mkosi.extra/root/
fi
mkdir -p mkosi.extra/root/bin
if [ -d ~/git/extra-scripts ]; then
rsync "${rsync_opts[@]}" ~/git/extra-scripts/bin/* mkosi.extra/root/bin/
fi
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if [[ "$_arg_extra_dirs" ]]; then
if [[ -n "$_arg_extra_dirs" ]]; then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we are not checking for the file here. See comment above about the lack of shell expansion for _arg_extra_dirs. This check is simply for the value being set. If it is set we must canonicalize it before we can check for the file. Probably worth a comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

-n is not checking for the file either. It's a string operator. It's more explicit, clearer and generally safer because variables can start with special characters (like -)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the quotes would save us from special characters... :-/ ok I'll add it. Sorry was thinking -f.

Copy link
Collaborator

@marc-hb marc-hb Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I thought the quotes would save us from special characters... :-/

They may or may not depending on which test instruction you use etc. It's complicated. I just keep it simple and always use -n.

Same with quotes: "When in doubt, double-quote every expansion in your shell commands."
https://mywiki.wooledge.org/Quotes#I.27m_Too_Lazy_to_Read.2C_Just_Tell_Me_What_to_Do

extra_dirs=$(make_canonical_path $_arg_extra_dirs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think make_canonical_path should not be used here, otherwise it's incompatible with shell completion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the contrary it is required for shell expansion. See comment about the if check above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shell expansion is a pretty big thing, a much bigger "feature" IMHO than "canonical_path". The function name should reflect that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok one could make the argument you are making. But the function returnes the fully qualified canonical path. That is it's purpose. I submit that passing "~/foo" should resolve to "/home//foo" and that is what the user expects. The fact that eval is used to do that is irrelavant to what the user expects.

Let me ask this another way. How else might "~" be expected to be interpreted if passed to this function?

BTW this also works through eval:

$HOME/foo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whatever happens what I don't want is to have to repeat this logic over and over for each path presented.

We could insist that all paths be absolute in the config file... Maybe that would be ok... dunno?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually like that idea. absolute paths only.

Copy link
Collaborator

@marc-hb marc-hb Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Let me ask this another way. How else might "~" be expected to be interpreted if passed to this function?

This is really not the question. The question is: "who knows what ELSE happens when using eval and shell expansion?" And the answer is: barely anyone!

BTW this also works through eval:

My point exactly! And many, many other things: https://www.gnu.org/software/bash/manual/html_node/Shell-Expansions.html
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html

We could insist that all paths be absolute in the config file..

If you mean: dropping shell expansion and eval then this would for sure simplify things tremendously. There is a reason why people say "eval is evil" :-)

But... dropping relative paths sounds like throwing the baby with the eval bathwater. A very easy way to deal with relative paths is to temporarily change directory back to whatever base you decide. Something like:

  for src_line in...
     ( cd $working_dir
        rsync -a "$src_line" ...
     )
  done

If a relative "$src_line" is not found, rsync will just fail with a useful error message.

if [[ ! -f $extra_dirs ]]; then
fail "$extra_dirs not found"
fi
echo "Installing extra directories and files from: $extra_dirs"
for d in `cat "$extra_dirs"`; do
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://www.shellcheck.net/wiki/SC2006

shellcheck -x run_qemu.sh

Don't forget the -x otherwise there is a ton of false positives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I always forget about shellcheck... I'll try it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Coding shell scripts without running shellcheck is now a crime :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW there are a few other warnings we should clean up if we are going to make that a requirement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Absolutely! But shellcheck can catch codestyle issues and sometimes bugs even long before it is requirement :-) This only requires keep the number of warnings to a reasonable level not to drown the new ones.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea probably better. cat just worked initially... :-D

d=$(make_canonical_path $d)
echo " $d"
if [[ -e $d ]]; then
rsync -a "$d" mkosi.extra/root/
else
fail "$d not found"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
fail "$d not found"
fail "extra-dir directory %s not found" "$d"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I missed that you are trying to use printf style here. Why?

Copy link
Collaborator

Choose a reason for hiding this comment

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

More flexible, safer, more consistent. But no big deal :-D

fi
done
fi

# timedatectl defaults to UTC when /etc/localtime is missing
local bld_tz; bld_tz=$( timedatectl | awk '/zone:/ { print $3 }' )
# v15 commit f11325afa02c "Adopt systemd-firstboot"
Expand Down