Don't rebuild daemon every time new git refs are fetched

[Serock3]

Fixes DES-950

---

This change is 

[linear]

DES-950 Daemon needs to be rebuilt whenever `.git/refs` changes

[faern]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Serock3)

---

mullvad-version/build.rs line 97 at r1 (raw file):

        .join("refs")
        .join("heads")
        .join(current_branch.trim());

Please mind detached head state. Seems like git branch --show-current just outputs nothing (empty string) in this case. I think this logic needs to be behind some if !current_branch.trim().is_empty() {...} because I guess the .exists() check will succeed because .git/refs/head exists and is a directory.

---

mullvad-version/build.rs line 110 at r1 (raw file):

            git_current_branch_ref.display()
        );
    }

get_dev_suffix now became significantly longer. Would it be possible to extract all this cargo:rerun-if-changed logic to a separate function(s)? Or some other way to massage this into shorter more dedicated functions.

[dlon]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dlon and @Serock3)

[Serock3]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @dlon and @faern)

---

mullvad-version/build.rs line 97 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Please mind detached head state. Seems like git branch --show-current just outputs nothing (empty string) in this case. I think this logic needs to be behind some if !current_branch.trim().is_empty() {...} because I guess the .exists() check will succeed because .git/refs/head exists and is a directory.

Good catch. I added the check. Note that.git/HEAD contains the current commit ref when in detached head state, so we don't need to track anyhing in .git/refs/heads/ to rebuild on e.g. adding new commits.

---

mullvad-version/build.rs line 110 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

get_dev_suffix now became significantly longer. Would it be possible to extract all this cargo:rerun-if-changed logic to a separate function(s)? Or some other way to massage this into shorter more dedicated functions.

I put it in a separate function and added a bunch of comments enplaning the reasoning for the files we track. I also added a section on the .git/packed-refs file, and why we don't need to track it

[faern]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @dlon and @Serock3)

---

mullvad-version/build.rs line 97 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Good catch. I added the check. Note that.git/HEAD contains the current commit ref when in detached head state, so we don't need to track anyhing in .git/refs/heads/ to rebuild on e.g. adding new commits.

You don't do any trimming. Have you verified this works as intended? I'm just afraid the git output can be a single newline or something like that.

---

mullvad-version/build.rs line 110 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

I put it in a separate function and added a bunch of comments enplaning the reasoning for the files we track. I also added a section on the .git/packed-refs file, and why we don't need to track it

Nice!

---

mullvad-version/build.rs line 98 at r2 (raw file):

/// the current branch (`.git/refs/heads/$current_branch`) and on changes to the ref of the current
/// release tag (`.git/refs/tags/$current_release_tag`).
fn rerun_if_git_ref_changed(release_tag: &String) -> Option<()> {

Nit. But more idiomatic to accept &str than &String when you just need to immutably borrow.

---

mullvad-version/build.rs line 145 at r2 (raw file):

    // this folder, however, as any changes to the current branch, 'detached HEAD' state or tags
    // will update the corresponding `.git/refs` file we are tracking, even if it had previously
    // been pruned.

Nit, but this comment really looks like it belongs to the return value, which I don't think is the case? Maybe separate them by one or two extra newlines?

---

mullvad-version/build.rs line 146 at r2 (raw file):

    // will update the corresponding `.git/refs` file we are tracking, even if it had previously
    // been pruned.
    Some(())

Can the return type here maybe be changed to Result<(), ...>? Option<()> is a bit weird. The only early return I can spot is the git branch subprocess. If you just remove the .ok() call you'll get the correct type directly. Returning Ok(()) looks more idiomatic and familiar than Some(()).

[Serock3]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @dlon and @faern)

---

mullvad-version/build.rs line 97 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

You don't do any trimming. Have you verified this works as intended? I'm just afraid the git output can be a single newline or something like that.

Miss on my part, of course the trimming should be done before the empty-check. Although after inspecting the output, it does not seem to contain a newline when in detached head state, so maybe the trim can be removed.

---

mullvad-version/build.rs line 110 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Nice!

Done.

---

mullvad-version/build.rs line 98 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Nit. But more idiomatic to accept &str than &String when you just need to immutably borrow.

Ah, I used the rust-analyzer code action "Extract into function" and didn't notice the @String. Good catch again!

---

mullvad-version/build.rs line 145 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Nit, but this comment really looks like it belongs to the return value, which I don't think is the case? Maybe separate them by one or two extra newlines?

Done

---

mullvad-version/build.rs line 146 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Can the return type here maybe be changed to Result<(), ...>? Option<()> is a bit weird. The only early return I can spot is the git branch subprocess. If you just remove the .ok() call you'll get the correct type directly. Returning Ok(()) looks more idiomatic and familiar than Some(()).

Done. But note that since we only use Option<T> as return types throughout the module we still have to call .ok() later on

[dlon]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @faern)

---

mullvad-version/build.rs line 78 at r3 (raw file):

    };

    rerun_if_git_ref_changed(&release_tag).ok()?;

Does this have to be a side-effect of calling get_dev_suffix?

[Serock3]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @dlon and @faern)

---

mullvad-version/build.rs line 78 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

Does this have to be a side-effect of calling get_dev_suffix?

No, I moved it to get_product_version, which ended up a bit cleaner.

[faern]

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @dlon)

---

mullvad-version/build.rs line 146 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Done. But note that since we only use Option<T> as return types throughout the module we still have to call .ok() later on

Of course! But we can still make each function as idiomatic as possible. Moving those things upwards to the level where they make the most sense.

[faern]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @dlon and @Serock3)

---

mullvad-version/build.rs line 78 at r3 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

No, I moved it to get_product_version, which ended up a bit cleaner.

This is fine, but I don't 100% agree it is better (no need to change anything).

Now the "dependencies" for computing the version string are moved further away from where they are used. If we now were to start depending on .git/foo/bar for our version computation it could be easy to forget to emit the rerun-if since it's in a different place to where the reading of the file happens.

---

mullvad-version/build.rs line 80 at r4 (raw file):

/// This also returns `None` if the `git` command can't run, or the code does
/// not live in a git repository.
fn get_dev_suffix(release_tag: &str) -> Option<String> {

The documentation for this function is now wrong, please update it 🙏

---

mullvad-version/build.rs line 134 at r4 (raw file):
Nit. Tags rarely move, so even if the comment is not wrong it reads strangely to me. In practice this would trigger if the release tag was created most of the time. How about something like:

Since the product version depends on if the build is done on the commit with the corresponding release tag or not, we must track creation of/changes to said tag

[Serock3]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dlon and @faern)

---

mullvad-version/build.rs line 146 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Of course! But we can still make each function as idiomatic as possible. Moving those things upwards to the level where they make the most sense.

I ended up taking this idea a bit further and also changed the return type of git_rev_parse_commit_hash from an Option<String> to just String and panicking on failed calls to git rev-parse. The previous behavior was to fail silently which ended up producing a release version string, which seemed wrong to me. This could be triggered if, e.g., the text in the product-version.txt file was changed to a tag that doesn't exist. Say that I change it to 2042.3 by mistake, then the computed version would just become 2042.3 regardless of the git ref of HEAD, instead of emitting an error.

---

mullvad-version/build.rs line 78 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This is fine, but I don't 100% agree it is better (no need to change anything).

Now the "dependencies" for computing the version string are moved further away from where they are used. If we now were to start depending on .git/foo/bar for our version computation it could be easy to forget to emit the rerun-if since it's in a different place to where the reading of the file happens.

I agree with your sentiment. I ended up removing and inlining the get_dev_suffix fn which simplified the code a bit and I added a comment about keeping rerun_if_git_ref_changed updated with git_rev_parse_commit_hash.

---

mullvad-version/build.rs line 80 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

The documentation for this function is now wrong, please update it 🙏

See comment above. I updated the text and moved it to a normal comment where get_dev_suffix previously was called.

[faern]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @dlon and @Serock3)

---

mullvad-version/build.rs line 146 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

I ended up taking this idea a bit further and also changed the return type of git_rev_parse_commit_hash from an Option<String> to just String and panicking on failed calls to git rev-parse. The previous behavior was to fail silently which ended up producing a release version string, which seemed wrong to me. This could be triggered if, e.g., the text in the product-version.txt file was changed to a tag that doesn't exist. Say that I change it to 2042.3 by mistake, then the computed version would just become 2042.3 regardless of the git ref of HEAD, instead of emitting an error.

Please mind that mullvad-version must still build even if git is not installed or if the .git directory is removed. So this change seems wrong to me

[faern]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @dlon and @Serock3)

---

mullvad-version/build.rs line 146 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Please mind that mullvad-version must still build even if git is not installed or if the .git directory is removed. So this change seems wrong to me

This is documented in the documentation for get_dev_suffix and that was the correct behavior. The code living in a git repository should not be a prerequisite for building the app.

[dlon]

Reviewed 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @faern and @Serock3)

---

mullvad-version/build.rs line 78 at r3 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

I agree with your sentiment. I ended up removing and inlining the get_dev_suffix fn which simplified the code a bit and I added a comment about keeping rerun_if_git_ref_changed updated with git_rev_parse_commit_hash.

In my opinion, functions with a get prefix should not have any meaningful side effects, such as emitting rerun-if-changed.

Since this is a small build script, I don't think the point is strong. But that's what I was initially hinting at.

[faern]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Serock3)

---

mullvad-version/build.rs line 78 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

In my opinion, functions with a get prefix should not have any meaningful side effects, such as emitting rerun-if-changed.

Since this is a small build script, I don't think the point is strong. But that's what I was initially hinting at.

If the naming is the issue here we can rename it to compute 🤷 I think a design that prevents easy mistakes and keep related things close together is more important.

[Serock3]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)

---

mullvad-version/build.rs line 146 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This is documented in the documentation for get_dev_suffix and that was the correct behavior. The code living in a git repository should not be a prerequisite for building the app.

Ok, but falling back to assuming a release tag seems like the wrong behavior to me. What about having a custom release tag for this scenario? "Dirty" (as in 2024.2-dev-dirty) is commonly used to indicate that there are modified files that are not commited, which is not quite correct in this case. Maybe 2024.2-dev-unversioned or 2024.2-dev-local-build?

---

mullvad-version/build.rs line 78 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

If the naming is the issue here we can rename it to compute 🤷 I think a design that prevents easy mistakes and keep related things close together is more important.

Renamed the fn to compute_product_version.

[dlon]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)

---

mullvad-version/build.rs line 78 at r3 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Renamed the fn to compute_product_version.

I'm ok with this, but, to be honest, changing the prefix to compute doesn't change anything

[Serock3]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dlon and @faern)

---

mullvad-version/build.rs line 78 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

I'm ok with this, but, to be honest, changing the prefix to compute doesn't change anything

Sure doesn't. Not sure if there are idiomatic names for non-pure functions 🤷. I updated the docstring to explicitly mention the side effects.

[Serock3]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dlon and @faern)

---

mullvad-version/build.rs line 146 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Ok, but falling back to assuming a release tag seems like the wrong behavior to me. What about having a custom release tag for this scenario? "Dirty" (as in 2024.2-dev-dirty) is commonly used to indicate that there are modified files that are not commited, which is not quite correct in this case. Maybe 2024.2-dev-unversion?

I ended up reverting this change and the one to git_rev_parse_commit_hash. Although I like the idea of validating that the release tag matches an existing tag and emitting a nice error otherwise, it caused problems with the CI where tags are not currently fetched. Fetching release tags also may not be a requirement we want to force on users that want to build the app. I did however add an explicit check for being in a valid repository, otherwise -dev-unknown is appended to the version. If the repo is valid but the release tag still cannot be found, we now default to appending a -dev-$hash suffix to make it slightly harder to accidentally produce something that looks like a release build.

[dlon]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)

[Serock3]

I did however add an explicit check for being in a valid repository, otherwise -dev-unknown is appended to the version.

I reverted this behavior, as per our discussion.

If the repo is valid but the release tag still cannot be found, we now default to appending a -dev-$hash suffix to make it slightly harder to accidentally produce something that looks like a release build.

This was how it worked from the start. The behavior is unchanged, sorry for the confusion on my part.

[faern]

Good job! I could have been clearer with the requirements of this file up front. This is probably going to be a good time saver with regards to compile times!

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Serock3)