Use git rev-parse in `justfile and other process

[yihuaf]

@utam0k I want to continue the discussion from this PR here:

#2027 (comment)

Specifically, I am of the opinion that removing git from the build dependency, especially the use of git rev-parse does not gain much for us.

First, I believe that git is ubiquitous and a requisite to a lot of our development workflow already. In the case of github action, git is definitely pre-installed and used to clone the repo. The only case I can think of where we have to install git is if we mount the source code into a container or vagrant vm to build youki, but even in these case installing git is not a big deal.

Second, git is already entrenched in the workflow. justfile was only a part. From integration_test.sh to build.sh, using git to locate the project root directory is common.

Third, git rev-parse is the only effective way of locating the project root from anywhere inside the project, independent of the cwd. This is helpful that scripts and justfiles don't have to keep track of where the project roots are. The alternative is to either force users to call justfile from the root of the project or we use relative path to the {{ justfile_directory }}. Using relative path can create trouble because ../../ can be bug prone.

Therefore, I would suggest us to keep using git rev-parse to obtain the project root wherever needed.

[utam0k]

Thanks for creating the RFC issue 🙏

Is this true? I don't think so because we are supposed to be using ubuntu:22.04 images.

In the case of github action, git is definitely pre-installed and used to clone the repo.

I'd like to know the concrete cases where the bug occurs with {{ justfile_directroy }}.

Using relative path can create trouble because ../../ can be bug prone.

[yihuaf]

I don't think so because we are supposed to be using ubuntu:22.04 images.

Just to double check, we are referring to the github action ubuntu 22 image right? It actually has a bunch of things pre-installed, including git. Reference: https://github.com/actions/runner-images/blob/main/images/linux/Ubuntu2204-Readme.md. It is not a stock/empty ubuntu 22 image.

I'd like to know the concrete cases where the bug occurs with {{ justfile_directroy }}.

Consider the we have two justfiles.

youki/justfile
# Either as individual justfile or `youki/justfile` calls `just -f crates/justfile XXX`.
youki/crates/justfile

Inside the youki/crate/justfile, we want to call youki/script/some_script.sh. There are two ways we can find the youki/script directory. One is {{ justfile_directory }}/../script/some_script.sh and another is:

root := `git rev-parse --show-toplevel`
{{ root }}/script/some_script.sh

If later, we want to move the youki/crates/justfile to youki/crates/libcontainer/justfile, we will have to make the relative path from {{ justfile_directory }}/../script/some_script.sh to {{ justfile_directory }}/../../script/some_script.sh. Constantly, we have to keep track of the relative path of where things are. It can be much easier to always deterministically find the project root. Also, from a readability perspective, ../../ is much harder to read compared to {{ root }}/ some_path.

In the end, this is not something that I have a lot of strong opinion on. I do think using git rev-parse for finding project root path is a tried and true technique, and it has some good pros without much cons. I also think trying to get rid of git from the dev dependencies is not a worthy goal to us.

[utam0k]

Oops, sorry. I misunderstood something. You are right 🙏

Just to double check, we are referring to the github action ubuntu 22 image right? It actually has a bunch of things pre-installed, including git. Reference: https://github.com/actions/runner-images/blob/main/images/linux/Ubuntu2204-Readme.md. It is not a stock/empty ubuntu 22 image.

Is this not good enough?

root := {{ justfile_directory() }}/../

I have a problem with this, but from a readability point of view, I think it is difficult to understand what the result of this git command itself will be.
I wonder why we bother to rely on git when there are alternative functions in just.

[yihuaf]

I think it is difficult to understand what the result of this git command itself will be.

Do you mean the git command git rev-parse --show-toplevel? If run inside a git repo, it finds the root directory of the git repo. You can run the command anywhere inside the repo to locate that root path. This is especially helpful in scripts where the location of the script any be anywhere inside the repo. This also helps when the location of the script changes, there are less things to refactor. In fact, we are already using this technique in many places such as the build.sh and integration_test.sh.

root := {{ justfile_directory() }}/../
I wonder why we bother to rely on git when there are alternative functions in just.

If you consider using relative path to be good enough, then I guess the argument is mute. From my perspective, using ../ is more error prone than using git.

Also note, if we only have a single justfile at the root of the project directory, then git rev-parse and {{ justfile_directory() }} are equivalent.

[utam0k]

Of course I know because I was the first to introduce this command. But an outside contributor would not know what this command is for a moment. It is an unfamiliar subcommand.

Do you mean the git command git rev-parse --show-toplevel? If run inside a git repo, it finds the root directory of the git repo. You can run the command anywhere inside the repo to locate that root path. This is especially helpful in scripts where the location of the script any be anywhere inside the repo. This also helps when the location of the script changes, there are less things to refactor. In fact, we are already using this technique in many places such as the build.sh and integration_test.sh.

What kind of case is it?

From my perspective, using ../ is more error prone than using git.

I am concerned that I cannot envision too many specific cases of error.

How about something like this? If you actually encounter a problem, introduce git rev-parse.

[yihuaf]

If you actually encounter a problem, introduce git rev-parse.

Sounds fair.

[utam0k]

@yihuaf Thanks for your understanding.