Too many "mkosi.extra/" cooks spoil the broth

[marc-hb]

As of today, git grep mkosi.extra | wc -l shows 36 hits. It's impossible to tell whether some lines potentially overwrite and generally clash with others.

Some rsync commands use --delete which is well intentioned (don't want one run to pollute the next) but is also a risk of conflict.

This needs a rethink, redesign and cleanup.

Originally posted by @marc-hb in #122 (comment)

I'm much more worried about the different rsync commands semi-randomly overwriting each other. I'll file a new bug.

[marc-hb]

When cleaning this up, it would be a good time to address the TODO in 936b03c

WARNING: --ndctl-build ignored when updating existing image! Outdated ndctl in the image. Try adding "-r img"

[marc-hb]

Some rsync commands use --delete which is well intentioned (don't want one run to pollute the next) but is also a risk of conflict.

rsync_opts makes other, debatable choices:

• --exclude=.git. Why?
• --copy-links, -L transform symlink into referent file/dir

To keep things simple, everything should be copied "as is" IMHO.

[weiny2]

Some rsync commands use --delete which is well intentioned (don't want one run to pollute the next) but is also a risk of conflict.

I think it is probably ok to drop the delete. one can always use -r wipe to clean the root. And one often does! ;-)

rsync_opts makes other, debatable choices:

* `--exclude=.git`. Why?

This is just a time saver I think. For the use case of testing changes in a user space program like ndctl we don't care about the git bits in the guest image. So save the time and don't copy them.

* `--copy-links, -L         transform symlink into referent file/dir`

Here you need to do this or the file the link points to may not (probably) won't exist...

OK I've done some testing on this feature and it is not working as I would like. I want to include rasdaemon and ras-tool in the guest image. But I don't want the packages. I want the git trees from my home directory. I do not want to clone the tree's directly in the run_qemu repo though. So I did this:

19:26:39 > pwd && ll
/home/iweiny/dev/run_qemu/mkosi/extra/root
total 12
drwxr-xr-x. 1 iweiny iweiny 72 Mar 6 19:17 ./
drwxr-xr-x. 1 iweiny iweiny 8 Mar 6 18:28 ../
lrwxrwxrwx. 1 iweiny iweiny 26 Mar 6 19:17 rasdaemon -> /home/iweiny/dev/rasdaemon/
lrwxrwxrwx. 1 iweiny iweiny 26 Mar 6 19:17 ras-tools -> /home/iweiny/dev/ras-tools/
-rwxr-xr-x. 1 iweiny iweiny 226 Mar 6 19:00 reinstall_ndctl.sh*

Which resulted in this in the guest:

19:26:43 > pwd && ll
/root
total 12
drwx------. 1 root root 44 Mar 6 19:24 bin
drwx------. 1 root root 602 Mar 6 19:24 ndctl
lrwxrwxrwx. 1 root root 26 Mar 6 19:24 ras-tools -> /home/iweiny/dev/ras-tools
lrwxrwxrwx. 1 root root 26 Mar 6 19:24 rasdaemon -> /home/iweiny/dev/rasdaemon
-rwx------. 1 root root 226 Mar 6 19:24 reinstall_ndctl.sh

there is no '/home/iweiny' in the guest... 😦

[marc-hb]

For the use case of testing changes in a user space program like ndctl we don't care about the git bits in the guest image. So save the time and don't copy them.

Unless someone wants to copy kernel sources, this does not save any measurable time when compared to the total build time.

Here you need to do this or the file the link points to may not (probably) won't exist...

As usual with symbolic links, "it depends". Symbolic links can also be relative and point to other files copied too. In that case it's best to avoid the duplication: imagine someone wants to make a quick modification while inside the image?

But I don't want the packages. I want the git trees from my home directory. I do not want to clone the tree's directly in the run_qemu repo though.

This seems useful but copying stuff outside the run_qemu.git repo is basically a different feature. As a coincidence this was discussed in the "extra-scripts" thread at #122 (comment)

I think what would be great is to replace the crude ~/git/extra-scripts hardcoding with an optional list of files passed to rsync -a --files-from=optional-list.txt ...

optional-list.txt can then point at anything you want.

cc: @stellarhopper

[weiny2]

Here you need to do this or the file the link points to may not (probably) won't exist...

As usual with symbolic links, "it depends". Symbolic links can also be relative and point to other files copied too. In that case it's best to avoid the duplication: imagine someone wants to make a quick modification while inside the image?

I don't quite follow why making a quick mod in the image makes a difference here.

But I don't want the packages. I want the git trees from my home directory. I do not want to clone the tree's directly in the run_qemu repo though.

This seems useful but copying stuff outside the run_qemu.git repo is basically a different feature. As a coincidence this was discussed in the "extra-scripts" thread at #122 (comment)

extra-scripts was also a hack. This is why I got on this yesterday because I'm wanting to add some ras stuff and I don't want to hard code things like with ndctl. I'm pushing run_qemu to be more generic...

I think what would be great is to replace the crude ~/git/extra-scripts hardcoding with an optional list of files passed to rsync -a --files-from=optional-list.txt ...

optional-list.txt can then point at anything you want.

I like this idea. I was also toying with the idea of an additional package list. But I don't know mkosi configs well enough to know the best way to do that.

Since I'm wishing for unicorns and ponies here! 😃 Perhaps we should read these config files from the kernel tree? 🤔

That way if you are working on different parts of the kernel the right user space stuff gets installed for the right kernel stuff you are working on. That is where extra-scripts was hacked from. When I worked on PKS I would install the pks test app in there as part of the kernel build. But that got changed with:

ef916f3 ("run_qemu: Add kernel selftests build option")

Since I'm needing this now let me get a patch together.

[weiny2]

I think what would be great is to replace the crude ~/git/extra-scripts hardcoding with an optional list of files passed to rsync -a --files-from=optional-list.txt ...
optional-list.txt can then point at anything you want.

I like this idea. I was also toying with the idea of an additional package list. But I don't know mkosi configs well enough to know the best way to do that.

Since I'm wishing for unicorns and ponies here! 😃 Perhaps we should read these config files from the kernel tree? 🤔

That way if you are working on different parts of the kernel the right user space stuff gets installed for the right kernel stuff you are working on. That is where extra-scripts was hacked from. When I worked on PKS I would install the pks test app in there as part of the kernel build. But that got changed with:

ef916f3 ("run_qemu: Add kernel selftests build option")

Since I'm needing this now let me get a patch together.

I've changed my mind. I think this is best handled as an option which the user can point wherever they want. They can manage a single file in their source trees and always point run_qemu there or whatever they come up with.

[marc-hb]

In that case it's best to avoid the duplication

I don't quite follow why making a quick mod in the image makes a difference here.

When you replace an (internal) symlink with a copy, now you need to duplicate any change you want to make. That's IMHO more error-prone and risky than not using --delete :-) "Bonus" points when the symlink used to point to a directory :-(

extra-scripts was also a hack.

Yes and @stellarhopper said in #122 that he would be happy for extra-scripts to be replaced by something better.

because I'm wanting to add some ras stuff and I don't want to hard code things like with ndctl. I'm pushing run_qemu to be more generic...

We all want more generic but compiling something will always be project-specific... ndctl is even more special because run_qemu knows how to run its tests. So at best run_qemu would need some per-project configuration files.

I was also toying with the idea of an additional package list. But I don't know mkosi configs well enough to know the best way to do that.

I don't remember seeing anything like what we are discussing right now. I mean nothing like "compiling and deploying from source". mkosi can install any distro package you want (and we of course already configure that), and it can also copy to the image anything you want: this is mkosi.extra. You can also run any script in the chroot for compilation or other - we also use that to compile ndctl. I'll take another look at mkosi documentation but I'm not holding my breath because this is already very generic and can do pretty much anything already so why would mkosi implement anything more specific and more complicated?

Perhaps we should read these config files from the kernel tree?

Mmmm... not sure why we should un-hardcode locations away from run_qemu.git to re-hardcode them to a location in a different git repo... Ideally, such config files could live anywhere? Maybe with some well chosen default location to save typing?

[marc-hb]

I'll take another look at mkosi documentation...

So, recent mkosi versions (which ones?) do have a BuildScripts= option that we could use...
https://github.com/systemd/mkosi/blob/v25/mkosi/resources/man/mkosi.1.md#scripts

... but this would complexify the flow and not bring anything to the table.

---

All the mkosi's XXXScripts= hooks exist because mkosi assumes it is the main user interface and runs "on top":

$ mkosi -> XXXScripts=

But run_qemu also assumes it is the main user interface:

$ run_qemu -> build_our_tests(), prepares mkosi.extra/
           -> mkosi, installs `mkosi.extra/` as is

Using BuildScripts= and friends would add one indirection level, which would make things more complex to debug:

$ run_qemu -> mkosi -> BuildScripts=build_our_tests(), installs in $DESTDIR

We would also have to fit into the mkosi Scripts= framework of the day and lose flexibility to prepare mkosi.extra in any way we want. This would again regularly break across mkosi versions.

[marc-hb]

optional list of files passed to rsync -a --files-from=optional-list.txt ...

I like(d) this idea.

My bad: after a closer look at --files-from=optional-list.txt I see now that it is only for paths relative to a top SRC argument. So, definitely not what you are looking for, which is why you did not use it in #125