Skip to content

Remove git2 dependency #934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

patrickjcasey
Copy link
Contributor

@patrickjcasey patrickjcasey commented Feb 18, 2025

Overview

This PR is split into 5 different commits to make it easier to review and revert, if there is a problem in the future. The commits are as follows:

  1. replace git2-based fetch implementation with gix-based implementation
  2. replace git2-based clone implementation with gix-based implementation
  3. replace git2-based checkout implementation with gix-based implementation
  4. remove git2 dependency and remaining git2 code (logging shim, rustls transport shim...)
  5. integrate gix functionality with user facing output, so progress is visible to user

Benefits

  • removed git2 and openssl dependencies
  • standardized on gix for git operations, as it is already in use with mitre/git plugin
  • considerable build time reduction (30% less time for debug build, 20% faster release builds)
  • enhanced git UI output to be more informative
  • faster hc check when a git clone is needed (~5% faster mitre/hipcheck, ~8% faster serde-rs/serde)

Comparison of User Facing Output

main git clone

main-git-clone

remove-git2-dependency git clone

remove-git2-clone

Build Timings

Note: All benchmarks were performed on an Ubuntu 24.04 machine with 8 CPUs and 32 GB of RAM

cargo build --workspace

Benchmark Command

cd /tmp && \
hyperfine --runs 2 \
    --prepare 'cd ~/workspace/hipcheck/main && cargo clean' \
    'cd ~/workspace/hipcheck/main && RUSTC_WRAPPER="" cargo build --workspace'  \
    --prepare 'cd ~/workspace/hipcheck/patrickjcasey/remove-git2-dependency && cargo clean' \
    'cd ~/workspace/hipcheck/patrickjcasey/remove-git2-dependency  &&  RUSTC_WRAPPER="" cargo build --workspace'

Results

image

cargo build --workspace --release

Benchmark Command

cd /tmp && \
hyperfine --runs 1 \
    --prepare 'cd ~/workspace/hipcheck/main && cargo clean' \
    'cd ~/workspace/hipcheck/main && RUSTC_WRAPPER="" cargo build --workspace --release'  \
    --prepare 'cd ~/workspace/hipcheck/patrickjcasey/remove-git2-dependency && cargo clean' \
    'cd ~/workspace/hipcheck/patrickjcasey/remove-git2-dependency  &&  RUSTC_WRAPPER="" cargo build --workspace --release'

Results

image

Cold Repo Analysis Benchmark

mitre/hipcheck cold start

testing a repo with ~700 commits

benchmark command

hyperfine --runs 3 \
    --prepare 'rm -rf ~/.cache/hipcheck/clones/github/mitre && cd ~/workspace/hipcheck/main' \
    './target/release/hc check --policy ./config/local.release.Hipcheck.kdl https://github.com/mitre/hipcheck'  \
    --prepare 'rm -rf ~/.cache/hipcheck/clones/github/mitre && cd ~/workspace/hipcheck/patrickjcasey/remove-git2-dependency ' \
    './target/release/hc check --policy ./config/local.release.Hipcheck.kdl https://github.com/mitre/hipcheck'

benchmark result

image

serde-rs/serde cold start

testing a repo with ~4000 commits

benchmark command

hyperfine --runs 3 \
    --prepare 'rm -rf ~/.cache/hipcheck/clones/github/serde-rs/serde&& cd ~/workspace/hipcheck/main' \
    './target/release/hc check --policy ./config/local.release.Hipcheck.kdl https://github.com/serde-rs/serde'  \
    --prepare 'rm -rf ~/.cache/hipcheck/clones/github/serde-rs/serde && cd ~/workspace/hipcheck/patrickjcasey/remove-git2-dependency ' \
    './target/release/hc check --policy ./config/local.release.Hipcheck.kdl https://github.com/serde-rs/serde'

benchmark result

image

numpy/numpy cold start

testing a repo with ~38000 commits

benchmark command

hyperfine --runs 3 \
    --prepare 'rm -rf ~/.cache/hipcheck/clones/github/numpy/numpy && cd ~/workspace/hipcheck/main' \
    './target/release/hc check --policy ./config/local.release.Hipcheck.kdl https://github.com/numpy/numpy'  \
    --prepare 'rm -rf ~/.cache/hipcheck/clones/github/numpy/numpy && cd ~/workspace/hipcheck/patrickjcasey/remove-git2-dependency ' \
    './target/release/hc check --policy ./config/local.release.Hipcheck.kdl https://github.com/numpy/numpy'

benchmark result

image

Manual testing performed

Since this change impacts the way repos are retrieved and/or updated, I performed the following manual tests to verify the new implementation is working correctly:

hc run requiring a clone

rm -rf ~/.cache/hipcheck/clones/github/rustls/rustls
./target/debug/hc check --policy ./config/local.Hipcheck.kdl https://github.com/rustls/rustls
# manually checked commit in  ~/.cache/hipcheck/clones/github/rustls/rustls matches commit hash of tip of main

hc run with existing repo, fetch & pull needed

cd  ~/.cache/hipcheck/clones/github/rustls/rustls
git reset --hard HEAD~10
./target/debug/hc check --policy ./config/local.Hipcheck.kdl https://github.com/rustls/rustls
# manually checked commit in  ~/.cache/hipcheck/clones/github/rustls/rustls matches commit hash of tip of main

Test PyPI Package without version

rm -rf ~/.cache/hipcheck/clones/github/tqdm
./target/debug/hc check --policy ./config/local.Hipcheck.kdl -t pypi tqdm
# manually checked commit in  ~/.cache/hipcheck/clones/github/tqdm/tqdm matches commit hash of latest version tag on github (4.67.1)

Test PyPI package with version

rm -rf ~/.cache/hipcheck/clones/github/tqdm
./target/debug/hc check --policy ./config/local.Hipcheck.kdl -t pypi [email protected]
# manually checked commit in  ~/.cache/hipcheck/clones/github/tqdm/tqdm matches commit hash of 4.50.0 tag on github

Test NPM Package without version

rm -rf ~/.cache/hipcheck/clones/github/tqdm
./target/debug/hc check --policy ./config/local.Hipcheck.kdl -t npm express
# manually checked commit in  ~/.cache/hipcheck/clones/github/expressjs/express matches commit hash of latest version tag on github (5.0.1)

Test NPM Package without version

rm -rf ~/.cache/hipcheck/clones/github/tqdm
./target/debug/hc check --policy ./config/local.Hipcheck.kdl -t npm [email protected]
# manually checked commit in  ~/.cache/hipcheck/clones/github/expressjs/express matches commit hash of 4.21.0 tag on github

@patrickjcasey patrickjcasey linked an issue Feb 18, 2025 that may be closed by this pull request
@patrickjcasey patrickjcasey added this to the 3.12.0 milestone Feb 18, 2025
@patrickjcasey patrickjcasey force-pushed the patrickjcasey/remove-git2-dependency branch 2 times, most recently from c77e930 to bf0943d Compare February 21, 2025 00:30
@alilleybrinker alilleybrinker changed the title WIP WIP: Remove git2 dependency Feb 25, 2025
@alilleybrinker alilleybrinker changed the title WIP: Remove git2 dependency Remove git2 dependency Feb 25, 2025
@alilleybrinker
Copy link
Collaborator

Updated title and removed WIP from it since it's already marked as a draft and thus can't be accidentally merged, even by myself or Julian.

@patrickjcasey patrickjcasey force-pushed the patrickjcasey/remove-git2-dependency branch 6 times, most recently from 81bd5e2 to ccd9945 Compare March 1, 2025 01:02
@patrickjcasey patrickjcasey marked this pull request as ready for review March 1, 2025 01:03
@j-lanson
Copy link
Collaborator

j-lanson commented Mar 3, 2025

Great work on this Patrick. I'm getting:

failed to clone remote repository
                    An IO error occurred when talking to the server

do you remember if there were any steps you needed to take to get gix to work with MITRE certs?

@j-lanson
Copy link
Collaborator

j-lanson commented Mar 3, 2025

I'm wondering if we don't have to do something like hipcheck/util/http/agent.rs

@j-lanson
Copy link
Collaborator

j-lanson commented Mar 3, 2025

Talking with @alilleybrinker , we feel that the issue is Gix is not picking up native cert store, and is using webpki instead. We need to figure out how to do that without re-introducing openssl

@patrickjcasey patrickjcasey force-pushed the patrickjcasey/remove-git2-dependency branch from ccd9945 to efcd4e7 Compare March 4, 2025 14:39
@patrickjcasey patrickjcasey marked this pull request as draft March 4, 2025 14:40
@patrickjcasey patrickjcasey force-pushed the patrickjcasey/remove-git2-dependency branch from 1cfcf07 to 58e004f Compare March 4, 2025 17:10
@patrickjcasey
Copy link
Contributor Author

Waiting for a response from the gitoxide project on the proper way of configuring the use of the system's certificates

@alilleybrinker
Copy link
Collaborator

Update: discussed during team sync today. Will continue waiting on guidance from the gitoxide folks rather than trying to maintain a patched fork with the support for certificate changes we need.

@alilleybrinker alilleybrinker modified the milestones: 3.12.0, 3.13.0 Mar 24, 2025
@j-lanson
Copy link
Collaborator

j-lanson commented Apr 9, 2025

Patrick and I were able to make forward progress on this. Basically, we configured gix to use the reqwest backend, which uses rust-tls. reqwest has a feature rustls-tls-native-roots to use the native root cert store, which does the exact same thing we do in util/http/agent.rs. In order to cause reqwest to compile with this feature that gix does not set, we added reqwest as a non-optional dependency to Hipcheck core. When cargo builds everything, cargo's feature unification will cause the reqwest package that gix depends on to contain this feature.

@patrickjcasey patrickjcasey force-pushed the patrickjcasey/remove-git2-dependency branch 3 times, most recently from 3da45f5 to 89424e2 Compare April 9, 2025 18:08
@patrickjcasey patrickjcasey force-pushed the patrickjcasey/remove-git2-dependency branch 5 times, most recently from c6f117e to 1acd373 Compare April 9, 2025 20:29
@patrickjcasey patrickjcasey marked this pull request as ready for review April 9, 2025 20:32
@patrickjcasey
Copy link
Contributor Author

The latest code is working on my MITRE Mac, meaning hipcheck is correctly using the system certificates

@patrickjcasey patrickjcasey force-pushed the patrickjcasey/remove-git2-dependency branch from 486d5da to 524bab8 Compare April 9, 2025 21:04
@patrickjcasey patrickjcasey enabled auto-merge April 9, 2025 21:32
@alilleybrinker alilleybrinker marked this pull request as draft April 15, 2025 16:02
auto-merge was automatically disabled April 15, 2025 16:02

Pull request was converted to draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Status: Todo
Development

Successfully merging this pull request may close these issues.

remove git2 dependency
3 participants