[RTE-710] elf2uki converter CLI tool #862
Conversation
|
|
||
| /// Run the `ukify` tool after validation of the passed-in arguments | ||
| fn build_uki(cli: &ValidatedCli, initramfs_path: &Path) -> Result<()> { | ||
| let mut command = Command::new(UKIFY_PATH); |
There was a problem hiding this comment.
So the README.md installation steps require you to create a virtual env directory (.venv) to install the ukify.py. But here, there is a direct invocation of UKIFY_PATH which would fail i believe if user does not invoke source .venv/bin/activate before doing cargo run.
There some alternatives, IMO better than what we have;
1- install ukify via package manager. For example, we as a company use ubuntu and ubuntu has systemd-ukify package. So you don't need to think about installing this scripts dependencies (pefile). You can invoke ukify just like here.
2- create a small project in build_init and create a small project there with uv package manager and a wrapper shell script. The wrapper shell script would do something like the following;
#!/bin/bash
PARSED_ARGS=()
parse_args() {
--linux) ..
--initrd)..
--stub)..
}
uv run --project . ./ukify.py "${PARSED_ARGS[@]}"
In general, elf2uki tool doesn't have to be a rust project. I think, if we add cli tool for initramfs crate, It could be python project or could even be a bash script.
There was a problem hiding this comment.
Discussed offline with Sinan and we'll assume ukify is installed externally for now.
| @@ -0,0 +1 @@ | |||
| target/ | |||
There was a problem hiding this comment.
i don't think this is necessary, its alread listed in <repo_root>/.gitignore.
There was a problem hiding this comment.
the repo root only ignored /target at the top-level; if you start a path with / it's anchored where the .gitignore is.
I didn't know why we did that so left it alone for now
There was a problem hiding this comment.
I removed this .gitignore again, because I added elf2uki to the top-level workspace, so it doesn't have its own target folder anymore
| help = "String to pass as the kernel command line", | ||
| value_name = "STRING" | ||
| )] | ||
| kernel_cmdline: Option<PathBuf>, |
There was a problem hiding this comment.
Shouldnt this be Option<String>?
There was a problem hiding this comment.
Also if this is a required value, shouldn't we remove Option<> type?
There was a problem hiding this comment.
It's not required; you can pass nothing and then the --cmdline argument will not be passed to ukify. It did need to be String, indeed; fixed.
| /// --kernel /tmp/kernel \ | ||
| /// --app /tmp/application \ | ||
| /// --uefi-stub /usr/lib/systemd/boot/efi/linuxx64.efi.stub \ | ||
| /// --cmdline "console=ttyS0 earlyprintk=serial" \ |
There was a problem hiding this comment.
I think we can make this value console=ttyS0 ... as default value, don't think that it would be changed often.
There was a problem hiding this comment.
I'm not sure we always want a cmdline value; this value is specifically good for debugging, so not sure we should default to it? It's fine to pass nothing; I removed the --cmdline from the example to illustrate that.
tvsfx
force-pushed
the
thomasvs/RTE-710-elf2uki
branch
from
edeb5ac to
c9310bb
Compare
tvsfx
force-pushed
the
thomasvs/RTE-710-elf2uki
branch
from
8ee6765 to
0b285d0
Compare
Taowyoo
left a comment
Taowyoo
left a comment
There was a problem hiding this comment.
except some nit pick, LGTM
Taowyoo
left a comment
Taowyoo
left a comment
There was a problem hiding this comment.
Since GH ubuntu has docker installed, could we add a separate GH job to test building this tool?
Ref: https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md
| initramfs_file = initramfs::build(application_elf, init, initramfs_file) | ||
| .context("failed to create initramfs")?; | ||
|
|
||
| build_uki(&validated_args, initramfs_file.path())?; |
There was a problem hiding this comment.
Nit: if the process exits with an error code or crashes the created temporary files are not deleted : https://docs.rs/tempfile/latest/tempfile/struct.NamedTempFile.html#resource-leaking.
This behaviour is beneficial if you have a failing ukify command and you want to give it a try manually.
There was a problem hiding this comment.
That's true, but I dont call exit(n) or similar in my code, nor do I expect anyone to run the binary with panic = 'abort', so I don't think a scenario where the \tmp files are not cleaned up will easily occur. I guess the main risk would be someone e.g. using Ctrl-C in the terminal. We could mitigate this by catching that signal (and potentially others), but that does seems like a pretty niche scenario
There was a problem hiding this comment.
This behaviour is beneficial if you have a failing ukify command and you want to give it a try manually.
If this turns out to be a problem, we can always make a persist flag or similar in the future, so that we don't delete the tmp files and log where they are.
No description provided.