Skip to content

Commit 7cba527

Browse files
committed
Auto merge of #11448 - Byron:integrate-gitoxide, r=weihanglo
gitoxide integration: fetch This PR is the first step towards resolving #1171. In order to get there, we integrate `gitoxide` into `cargo` in such a way that one can control its usage in nightly via `-Zgitoxide` or `Zgitoxide=<feature>[,featureN]`. Planned features are: * **fetch** - all fetches are done with `gitxide` (this PR) * **shallow_index** - the crates index will be a shallow clone (_planned_) * **shallow_deps** - git dependencies will be a shallow clone (_planned_) * **checkout** - plain checkouts with `gitoxide` (_planned_) The above list is a prediction and might change as we understand the requirements better. ### Testing and Transitioning By default, everything stays as is. However, relevant tests can be re-runwith `gitoxide` using ``` RUSTFLAGS='--cfg always_test_gitoxide' cargo test git ``` There are about 200 tests with 'git' in their name and I plan to enable them one by one. That way the costs for CI stay managable (my first measurement with one test was 2min 30s), while allowing to take one step at a time. Custom tests shall be added once we realize that more coverage is needed. That way we should be able to maintain running `git2` and `gitoxide` side by side until we are willing to switch over to `gitoxide` entirely on stable cargo. Then turning on `git2` might be a feature toggle for a while until we finally remove it from the codebase. _Please see the above paragraph as invitation for discussion, it's merely a basis to explore from and improve upon._ ### Tasks * [x] add feature toggle * [x] setup test system with one currently successful test * [x] implement fetch with `gitoxide` (MVP) * [x] fetch progress * [x] detect spurious errors * [x] enable as many git tests as possible (and ignore what's not possible) * [x] fix all git-related test failures (except for 1: built-in upload-pack, skipped for now) * [x] validate that all HTTP handle options that come from `cargo` specific values are passed to `gitoxide` * [x] a test to validate `git2` code can handle crates-index clones created with `gitoxide` and vice-versa * [x] remove patches that enabled `gitoxide` enabled testing - it's not used anymore * [x] ~~remove all TODOs and use crates-index version of `git-repository`~~ The remaining 2 TODO's are more like questions for the reviewer. * [x] run all tests with gitoxide on the fastest platform as another parallel task * [x] switch to released version * [x] [Tasks from first review round](#11448 (comment)) * [x] create a new `gitoxide` release and refer to the latest version from crates.io (instead of git-dependency) * [x] [address 2nd review round comments](#11448 (comment)) ### Postponed Tasks I suggest to go breadth-first and implement the most valuable features first, and then aim for a broad replacement of `git2`. What's left is details and improved compatibility with the `git2` implementation that will be required once `gitoxide` should become the default implementation on stable to complete the transition. * **built-in support for serving the `file` protocol** (i.e. without using `git`). Simple cases like `clone` can probably be supported quickly, `fetch` needs more work though due to negotiation. * SSH name fallbacks via a native (probably ~~libssh~~ (avoid LGPL) `libssh2` based) transport. Look at [this issue](#2399) for some history. * additional tasks from [this tracking issue](GitoxideLabs/gitoxide#450 (comment)) ### Proposed Workflow I am now using [stacked git](https://stacked-git.github.io) to keep commits meaningful during development. This will also mean that before reviews I will force-push a lot as changes will be bucketed into their respective commits. Once review officially begins I will stop force-pushing and create small commits to address review comments. That way it should be easier to understand how things change over time. Those review-comments can certainly be squashed into one commit before merging. _Please let me know if this is feasible or if there are other ways of working you prefer._ ### Development notes * unrelated: [this line](https://github.com/rust-lang/cargo/blob/9827412fee4f5a88ac85e013edd954b2b63f399b/src/cargo/ops/registry.rs#L620) refers to an issue that has since been resolved in `curl`. * Additional tasks related to a correct fetch implementation are collected in this [tracking issue](GitoxideLabs/gitoxide#450). **These affect how well the HTTP transport can be configured, needs work** * _authentication_ [is quite complex](https://github.com/rust-lang/cargo/blob/37cad5bd7f7dcd2f6d3e45312a99a9d3eec1e2a0/src/cargo/sources/git/utils.rs#L490) and centred around making SSH connections work. This feature is currently the weakest in `gitoxide` as it simply uses `ssh` (the program) and calls it a day. No authentication flows are supported there yet and the goal would be to match `git` there at least (which it might already do by just calling `ssh`). Needs investigation. Once en-par with `git` I think `cargo` can restart the whole fetch operation to try different user names like before. - the built-in `ssh`-program based transport can now understand permission-denied errors, but the capability isn't used after all since a builtin ssh transport is required. * It would be possible to implement `git::Progress` and just ignore most of the calls, but that's known to be too slow as the implementation assumes a `Progress::inc()` call is as fast as an atomic increment and makes no attempt to reduce its calls to it. * learning about [a way to get custom traits in `thiserror`](dtolnay/thiserror#212) could make spurious error checks nicer and less error prone during maintenance. It's not a problem though. * I am using `RUSTFLAGS=--cfg` to influence the entire build and unit-tests as environment variables didn't get through to the binary built and run for tests. ### Questions * The way `gitoxide` is configured the user has the opportunity to override these values using more specific git options, for example using url specific http settings. This looks like a feature to me, but if it's not `gitoxide` needs to provide a way to disable applying these overrides. Please let me know what's desired here - my preference is to allow overrides. * `gitoxide` currently opens repositories similar to how `git` does which respects git specific environment variables. This might be a deviation from how it was before and can be turned off. My preference is to see it as a feature. ### Prerequisite PRs * #11602
2 parents c61b0f0 + cfffda9 commit 7cba527

File tree

17 files changed

+856
-108
lines changed

17 files changed

+856
-108
lines changed

.github/workflows/main.yml

+24-3
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ jobs:
7474
os: windows-latest
7575
rust: stable-msvc
7676
other: i686-pc-windows-msvc
77-
- name: Windows x86_64 gnu nightly
77+
- name: Windows x86_64 gnu nightly # runs out of space while trying to link the test suite
7878
os: windows-latest
7979
rust: nightly-gnu
8080
other: i686-pc-windows-gnu
@@ -102,7 +102,16 @@ jobs:
102102
if: "!contains(matrix.rust, 'stable')"
103103

104104
- run: cargo test
105-
# The testsuite generates a huge amount of data, and fetch-smoke-test was
105+
- name: Clear intermediate test output
106+
run: |
107+
df -h
108+
rm -rf target/tmp
109+
df -h
110+
- name: gitoxide tests (all git-related tests)
111+
run: cargo test git
112+
env:
113+
__CARGO_USE_GITOXIDE_INSTEAD_OF_GIT2: 1
114+
# The testsuite generates a huge amount of data, and fetch-smoke-test was
106115
# running out of disk space.
107116
- name: Clear test output
108117
run: |
@@ -156,6 +165,18 @@ jobs:
156165
- run: rustup update stable && rustup default stable
157166
- run: cargo test --manifest-path crates/resolver-tests/Cargo.toml
158167

168+
test_gitoxide:
169+
runs-on: ubuntu-latest
170+
steps:
171+
- uses: actions/checkout@v3
172+
- run: rustup update --no-self-update stable && rustup default stable
173+
- run: rustup target add i686-unknown-linux-gnu
174+
- run: sudo apt update -y && sudo apt install gcc-multilib libsecret-1-0 libsecret-1-dev -y
175+
- run: rustup component add rustfmt || echo "rustfmt not available"
176+
- run: cargo test
177+
env:
178+
__CARGO_USE_GITOXIDE_INSTEAD_OF_GIT2: 1
179+
159180
build_std:
160181
runs-on: ubuntu-latest
161182
env:
@@ -196,7 +217,7 @@ jobs:
196217
permissions:
197218
contents: none
198219
name: bors build finished
199-
needs: [docs, rustfmt, test, resolver, build_std]
220+
needs: [docs, rustfmt, test, resolver, build_std, test_gitoxide]
200221
runs-on: ubuntu-latest
201222
if: "success() && github.event_name == 'push' && github.ref == 'refs/heads/auto-cargo'"
202223
steps:

Cargo.toml

+2
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ filetime = "0.2.9"
3030
flate2 = { version = "1.0.3", default-features = false, features = ["zlib"] }
3131
git2 = "0.16.0"
3232
git2-curl = "0.17.0"
33+
gix = { version = "0.38.0", default-features = false, features = ["blocking-http-transport-curl", "progress-tree"] }
34+
gix-features-for-configuration-only = { version = "0.27.0", package = "gix-features", features = [ "parallel" ] }
3335
glob = "0.3.0"
3436
hex = "0.4"
3537
hmac = "0.12.1"

crates/cargo-test-support/src/git.rs

+7
Original file line numberDiff line numberDiff line change
@@ -247,3 +247,10 @@ pub fn tag(repo: &git2::Repository, name: &str) {
247247
false
248248
));
249249
}
250+
251+
/// Returns true if gitoxide is globally activated.
252+
///
253+
/// That way, tests that normally use `git2` can transparently use `gitoxide`.
254+
pub fn cargo_uses_gitoxide() -> bool {
255+
std::env::var_os("__CARGO_USE_GITOXIDE_INSTEAD_OF_GIT2").map_or(false, |value| value == "1")
256+
}

src/cargo/core/features.rs

+84
Original file line numberDiff line numberDiff line change
@@ -716,6 +716,8 @@ unstable_cli_options!(
716716
doctest_xcompile: bool = ("Compile and run doctests for non-host target using runner config"),
717717
dual_proc_macros: bool = ("Build proc-macros for both the host and the target"),
718718
features: Option<Vec<String>> = (HIDDEN),
719+
gitoxide: Option<GitoxideFeatures> = ("Use gitoxide for the given git interactions, or all of them if no argument is given"),
720+
jobserver_per_rustc: bool = (HIDDEN),
719721
minimal_versions: bool = ("Resolve minimal dependency versions instead of maximum"),
720722
direct_minimal_versions: bool = ("Resolve minimal dependency versions instead of maximum (direct dependencies only)"),
721723
mtime_on_use: bool = ("Configure Cargo to update the mtime of used files"),
@@ -827,6 +829,74 @@ where
827829
parse_check_cfg(crates.into_iter()).map_err(D::Error::custom)
828830
}
829831

832+
#[derive(Debug, Copy, Clone, Default, Deserialize)]
833+
pub struct GitoxideFeatures {
834+
/// All fetches are done with `gitoxide`, which includes git dependencies as well as the crates index.
835+
pub fetch: bool,
836+
/// When cloning the index, perform a shallow clone. Maintain shallowness upon subsequent fetches.
837+
pub shallow_index: bool,
838+
/// When cloning git dependencies, perform a shallow clone and maintain shallowness on subsequent fetches.
839+
pub shallow_deps: bool,
840+
/// Checkout git dependencies using `gitoxide` (submodules are still handled by git2 ATM, and filters
841+
/// like linefeed conversions are unsupported).
842+
pub checkout: bool,
843+
/// A feature flag which doesn't have any meaning except for preventing
844+
/// `__CARGO_USE_GITOXIDE_INSTEAD_OF_GIT2=1` builds to enable all safe `gitoxide` features.
845+
/// That way, `gitoxide` isn't actually used even though it's enabled.
846+
pub internal_use_git2: bool,
847+
}
848+
849+
impl GitoxideFeatures {
850+
fn all() -> Self {
851+
GitoxideFeatures {
852+
fetch: true,
853+
shallow_index: true,
854+
checkout: true,
855+
shallow_deps: true,
856+
internal_use_git2: false,
857+
}
858+
}
859+
860+
/// Features we deem safe for everyday use - typically true when all tests pass with them
861+
/// AND they are backwards compatible.
862+
fn safe() -> Self {
863+
GitoxideFeatures {
864+
fetch: true,
865+
shallow_index: false,
866+
checkout: true,
867+
shallow_deps: false,
868+
internal_use_git2: false,
869+
}
870+
}
871+
}
872+
873+
fn parse_gitoxide(
874+
it: impl Iterator<Item = impl AsRef<str>>,
875+
) -> CargoResult<Option<GitoxideFeatures>> {
876+
let mut out = GitoxideFeatures::default();
877+
let GitoxideFeatures {
878+
fetch,
879+
shallow_index,
880+
checkout,
881+
shallow_deps,
882+
internal_use_git2,
883+
} = &mut out;
884+
885+
for e in it {
886+
match e.as_ref() {
887+
"fetch" => *fetch = true,
888+
"shallow-index" => *shallow_index = true,
889+
"shallow-deps" => *shallow_deps = true,
890+
"checkout" => *checkout = true,
891+
"internal-use-git2" => *internal_use_git2 = true,
892+
_ => {
893+
bail!("unstable 'gitoxide' only takes `fetch`, 'shallow-index', 'shallow-deps' and 'checkout' as valid inputs")
894+
}
895+
}
896+
}
897+
Ok(Some(out))
898+
}
899+
830900
fn parse_check_cfg(
831901
it: impl Iterator<Item = impl AsRef<str>>,
832902
) -> CargoResult<Option<(bool, bool, bool, bool)>> {
@@ -879,6 +949,13 @@ impl CliUnstable {
879949
for flag in flags {
880950
self.add(flag, &mut warnings)?;
881951
}
952+
953+
if self.gitoxide.is_none()
954+
&& std::env::var_os("__CARGO_USE_GITOXIDE_INSTEAD_OF_GIT2")
955+
.map_or(false, |value| value == "1")
956+
{
957+
self.gitoxide = GitoxideFeatures::safe().into();
958+
}
882959
Ok(warnings)
883960
}
884961

@@ -967,6 +1044,13 @@ impl CliUnstable {
9671044
"doctest-xcompile" => self.doctest_xcompile = parse_empty(k, v)?,
9681045
"doctest-in-workspace" => self.doctest_in_workspace = parse_empty(k, v)?,
9691046
"panic-abort-tests" => self.panic_abort_tests = parse_empty(k, v)?,
1047+
"jobserver-per-rustc" => self.jobserver_per_rustc = parse_empty(k, v)?,
1048+
"gitoxide" => {
1049+
self.gitoxide = v.map_or_else(
1050+
|| Ok(Some(GitoxideFeatures::all())),
1051+
|v| parse_gitoxide(v.split(',')),
1052+
)?
1053+
}
9701054
"host-config" => self.host_config = parse_empty(k, v)?,
9711055
"target-applies-to-host" => self.target_applies_to_host = parse_empty(k, v)?,
9721056
"publish-timeout" => self.publish_timeout = parse_empty(k, v)?,

src/cargo/ops/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ mod cargo_uninstall;
5252
mod common_for_install_and_uninstall;
5353
mod fix;
5454
mod lockfile;
55-
mod registry;
55+
pub(crate) mod registry;
5656
mod resolve;
5757
pub mod tree;
5858
mod vendor;

src/cargo/ops/registry.rs

+5-6
Original file line numberDiff line numberDiff line change
@@ -619,9 +619,6 @@ pub fn configure_http_handle(config: &Config, handle: &mut Easy) -> CargoResult<
619619
handle.useragent(&format!("cargo {}", version()))?;
620620
}
621621

622-
// Empty string accept encoding expands to the encodings supported by the current libcurl.
623-
handle.accept_encoding("")?;
624-
625622
fn to_ssl_version(s: &str) -> CargoResult<SslVersion> {
626623
let version = match s {
627624
"default" => SslVersion::Default,
@@ -631,13 +628,15 @@ pub fn configure_http_handle(config: &Config, handle: &mut Easy) -> CargoResult<
631628
"tlsv1.2" => SslVersion::Tlsv12,
632629
"tlsv1.3" => SslVersion::Tlsv13,
633630
_ => bail!(
634-
"Invalid ssl version `{}`,\
635-
choose from 'default', 'tlsv1', 'tlsv1.0', 'tlsv1.1', 'tlsv1.2', 'tlsv1.3'.",
636-
s
631+
"Invalid ssl version `{s}`,\
632+
choose from 'default', 'tlsv1', 'tlsv1.0', 'tlsv1.1', 'tlsv1.2', 'tlsv1.3'."
637633
),
638634
};
639635
Ok(version)
640636
}
637+
638+
// Empty string accept encoding expands to the encodings supported by the current libcurl.
639+
handle.accept_encoding("")?;
641640
if let Some(ssl_version) = &http.ssl_version {
642641
match ssl_version {
643642
SslVersionConfig::Single(s) => {

src/cargo/sources/git/mod.rs

+5
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
pub use self::source::GitSource;
22
pub use self::utils::{fetch, GitCheckout, GitDatabase, GitRemote};
33
mod known_hosts;
4+
mod oxide;
45
mod source;
56
mod utils;
7+
8+
pub mod fetch {
9+
pub type Error = gix::env::collate::fetch::Error<gix::refspec::parse::Error>;
10+
}

0 commit comments

Comments
 (0)