fix(libcontainer): Run test_is_executable with a more common file

[Overflow0xFFFF]

This test was failing on some popular Linux distros, like Fedora, because /boot/initrd.img doesn't exist. This change alters the test so that a more common file, /etc/hosts, is used.

After searching on the internet, there doesn't seem to be any one file that is always guaranteed to exist, other than the root filesystem path /. But /etc/hosts is very, very common, so I'm hopeful that this change means that the Youki build environment is more likely to build and pass its tests on more systems.

Relates to #1650.

[yihuaf]

Drive by comment: Is it possible to detect the distro (Ubuntu vs. Fedora) in the test and choose the path accordingly? In this way, we can provide a warning to distro we did not explicitly test.

I will let @YJDoc2 review the specifics in this PR :)

[YJDoc2]

Hey @Overflow0xFFFF Thanks for the PR, I'll take a look at this by today 👍

[Overflow0xFFFF]

Drive by comment: Is it possible to detect the distro (Ubuntu vs. Fedora) in the test and choose the path accordingly? In this way, we can provide a warning to distro we did not explicitly test.

I will let @YJDoc2 review the specifics in this PR :)

I think so. Most distros ship some kind of variant on the LSB Release file and have a utility / script that can parse that information. cargo test doesn't allow printing any messages without the --nocapture flag passed from the CLI, but that's not hard to fix. However, running the tests in parallel, the user might miss the message output in a sea of other messages.

If our goal is to notify the user, we could probably just add that logic to the Makefile, though. This would be easy to maintain in shell script format.

A third alternative would be to create a non-executable file ourselves and avoid parsing any distro-specific info anyway! I need to brush up some more on the context for the test, though. I was merely fixing a test that was broken in my workspace, so creating a file as part of the test suite could either be normal or overkill. Some software developers also don't like touching the filesystem as part of unit tests. If that's a philosophy that Youki's developers share, I don't want to intrude. 😄

Separately, would there be any interest in adding Rust builds on other OSes to the GitHub Actions workflow? And does Youki have certain supported distros in mind, or is it too early to ask that question?

[YJDoc2]

I think so. Most distros ship some kind of variant on the LSB Release file and have a utility / script that can parse that information. cargo test doesn't allow printing any messages without the --nocapture flag passed from the CLI, but that's not hard to fix. However, running the tests in parallel, the user might miss the message output in a sea of other messages.

I don't think warning users in test is particularly effective, I might suggest simply marking the test as passed + making sure that it always runs in the CI (i.e. whatever we use is SURE to be present in CI) so that we don't miss it in PRs.
I think we already check for OS info in the youki info , we might be able to get the distro from there.

A third alternative would be to create a non-executable file ourselves and avoid parsing any distro-specific info anyway!

This would be much preferable than parsing distro info, as long as we can guarantee that the file generated is cleaned up afterwards, irrespective of pass/fail. Youki's test already need mutable access to system in order to test certain cgroup stuff, so creating a file (as long as it is cleaned up) is not that big of an issue. I believe we have this kind of always-clean up logic for cgroups tests, we can work with that.

Separately, would there be any interest in adding Rust builds on other OSes to the GitHub Actions workflow?

I think we would like to validate support to other distros in CI, but we will need more discussion on this, specifically with regard to how to do it, is it worth to do it (as we mostly deal with linux , not specifically any distro related stuff), and if so to what extent we want to validate (just build, build+unit tests , build+integration tests)

[YJDoc2]

@Overflow0xFFFF , Overall this PR is good, I have added a comment please take a look.

If you think creating a file and testing it is better, and willing to take a try at it in this PR, we can go that way ; or else you'd rather have this merged, and not work on that / work on it later , I can approve and merge this.

Let me know, and thanks a lot for the contribution!

[Overflow0xFFFF]

@YJDoc2, I apologize for the delay. I was attempting a few solutions before throwing them out and settling on a simpler one. I'm not completely satisfied with the code as-written because I think I used unwrap() a little bit too much, so I'm really open to feedback on if there's a better way to handle the temporary file in my latest commit. 🙂

[YJDoc2]

Hey @Overflow0xFFFF , thanks for the update. I think a bit better approach for temp file wold be to use TempDir structure, and create a file in that. That dir deletes itself in Drop , so we can be assured it gets cleaned up.

It is defined in https://github.com/containers/youki/blob/main/crates/libcontainer/src/utils.rs#L279 and you can see the example usage in https://github.com/containers/youki/blob/main/crates/libcontainer/src/rootless.rs#L450

Can you take a look and use it instead? Also can I ask you to edit the last commit and force push so we have a nice commit history?
Thanks a lot!

[Overflow0xFFFF]

@YJDoc2, thank you for the links and advice! I have put TempDir to good use, and I've updated the test to rely on that instead. If there's anything else that's worth updating, changing, or tweaking, let me know!

[YJDoc2]

Hey, the CI is failing, which is a known issue 😓 but there are some more nit picks 😅

Thanks for updating the method to use TempDir, that makes cleaning it up that much better. Unfortunately, the way I suggested you to use it was wrong :(

The way it is used in the rootless is not optimal, as we have to manually pick out the base temp dir path from env etc. making it that much harder to change the approach if needed. There is already a function at https://github.com/containers/youki/blob/main/crates/libcontainer/src/utils.rs#L325 which will check the temp path from env, and use that to create a temp dir. That way if we have to change the approach in future, we have to change it in only one place.

Ideally the rootless test I mentioned should also do it that way, but I think it doesn't as it needs the directory in specific place.

Can I ask you to use the above function instead? Sorry for requesting all these changes, I should have done a better work of mentioning how to use the TempDir and looked at the rootless eg more 😓

Thanks :)

[Overflow0xFFFF]

No worries about the nitpicks! I completely understand, and I thank you for your patience and all the extra review time you've put in!

I grepped the codebase for other examples of create_temp_dir() so that this test creates a temporary directory in the same way. This hopefully looks like any other test in the code base!

[YJDoc2]

lgtm 👍
Thanks for your contributions!