update submodule libgit2 to new commit to fix 'omits non-default port numbers' #421

bhamail · 2019-05-17T19:38:07Z

Update submodule commit to use fixed version of libgit2 - #405

…ault port numbers' - rust-lang#405

bhamail · 2019-05-17T19:59:58Z

Hm. Not sure what is causing the "Some checks were not successful" failures.

In the libgit2-sys/libgit2 submodule folder, I ran git checkout 4ef2b889a.
Back in the top level folder I ran cargo test, and all was well.

Perhaps the CI builds should run cargo clean first?

ehuss · 2019-05-18T02:30:47Z

@bhamail There is a separate package that needs to be run separately (cargo run --manifest-path systest/Cargo.toml). You can see the commands that run during CI in the config (https://github.com/rust-lang/git2-rs/blob/master/ci/azure-test-all.yml).

Also, I would expect it would be good to update to the latest release, and there will likely be some work necessary to deal with any breakage and build issues.

bhamail · 2019-05-20T16:00:32Z

@ehuss Thanks for looking this over. I tried to drill into the cause of the failures of the systest command, but I'm stuck. Here's the error (w/ backtrace). FWIW, I did a cargo clean on both the top level project and the submodule before this error.

The details of the CI build failures seem to be showing this same error, so maybe we'll get lucky and fixing this error will fix them all.

export RUST_BACKTRACE=1
cargo run --manifest-path systest/Cargo.toml
...
    Finished dev [unoptimized + debuginfo] target(s) in 2.78s
     Running `target/debug/systest`
RUNNING ALL TESTS
bad GIT_DIFF_INDENT_HEURISTIC value at byte 2: rust: 0 (0x0) != c 4 (0x4)
bad GIT_DIFF_INDENT_HEURISTIC value at byte 3: rust: 128 (0x80) != c 0 (0x0)
thread 'main' panicked at 'some tests failed', /Users/bhamail/sonatype/community/rust/git2-rs/target/debug/build/systest-05071433ad5c0557/out/all.rs:13:21
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:71
   2: std::panicking::default_hook::{{closure}}
             at src/libstd/sys_common/backtrace.rs:59
             at src/libstd/panicking.rs:211
   3: std::panicking::default_hook
             at src/libstd/panicking.rs:227
   4: <std::panicking::begin_panic::PanicPayload<A> as core::panic::BoxMeUp>::get
             at src/libstd/panicking.rs:491
   5: std::panicking::begin_panic
             at /rustc/9fda7c2237db910e41d6a712e9a2139b352e558b/src/libstd/panicking.rs:425
   6: systest::main
             at ./target/debug/build/systest-05071433ad5c0557/out/all.rs:13
   7: std::rt::lang_start::{{closure}}
             at /rustc/9fda7c2237db910e41d6a712e9a2139b352e558b/src/libstd/rt.rs:74
   8: std::panicking::try::do_call
             at src/libstd/rt.rs:59
             at src/libstd/panicking.rs:310
   9: panic_unwind::dwarf::eh::read_encoded_pointer
             at src/libpanic_unwind/lib.rs:102
  10: <std::panicking::begin_panic::PanicPayload<A> as core::panic::BoxMeUp>::get
             at src/libstd/panicking.rs:289
             at src/libstd/panic.rs:398
             at src/libstd/rt.rs:58
  11: std::rt::lang_start
             at /rustc/9fda7c2237db910e41d6a712e9a2139b352e558b/src/libstd/rt.rs:74
  12: <usize as systest::Pretty>::pretty

I'm not sure where to dig next.

Re 'latest release': I did initially try updating to the latest libgit2 release, but ran into some build errors that I was unable to fix the first few times around. If we could get this smaller patch merged, that would resolve the current log jam. I'm willing to come back and work on updating to the latest release after that (w/ the assumption I'll be pestering you with more of my silly noob questions).

ehuss · 2019-05-21T17:49:23Z

The ctest is showing that the GIT_DIFF_INDENT_HEURISTIC const value has changed to 1 << 18. In libgit2-sys/lib.rs that constant value needs to change.

Also, I'm getting a seg fault in the all test. It looks like due to the introduction of custom allocators, git2_curl::register needs to call libgit2_sys::init first. I don't immediately see anything else that needs to do that, hopefully there is enough coverage in the tests to ensure that.

@ehuss

…ion in libgit2-sys/libgit2/include/git2/diff.h Thanks @ehuss

bhamail · 2019-05-21T21:29:05Z

Zoinks! It appears the CI build is happy now. Thanks @ehuss for the help!

alexcrichton · 2019-05-22T13:54:56Z

git2-curl/tests/all.rs

@@ -15,6 +15,7 @@ const PORT: u16 = 7848;

 fn main() {
    unsafe {
+        libgit2_sys::init();


This seems somewhat suspicious in the sense that git2_curl should internally not require this but instead do it on behalf of libraries. I think that there's perhaps a missing call to initialization somewhere inside of git2 itself, since git2_curl only uses the APIs of git2?

Yea, I meant that init should be called from git2_curl::register, not with.

I moved the init call into git2_curl::register. 69334f8
Feels like I'm missing a way to get this init'd without adding a direct dependency on libgit2_sys, but I'm not seeing it. (noob alert).

ehuss · 2019-05-22T15:28:23Z

git2-curl/Cargo.toml

@@ -23,6 +23,7 @@ civet = "0.11"
 conduit = "0.8"
 conduit-git-http-backend = "0.8"
 tempfile = "3.0"
+libgit2-sys = { path = "../libgit2-sys"}


This won't publish without a version.

Done in 437e22e

For my own learning: I was guessing a version was not needed due to no version declared in this other case for 'systest':
https://github.com/rust-lang/git2-rs/blob/master/systest/Cargo.toml#L8

I guess there is no magic that inherits the version from the higher level https://github.com/rust-lang/git2-rs/blob/master/Cargo.toml#L26 ?

ehuss · 2019-05-22T15:28:37Z

libgit2-sys/lib.rs

@@ -1112,7 +1112,7 @@ pub const GIT_DIFF_SHOW_UNMODIFIED: git_diff_option_t = 1 << 26;
 pub const GIT_DIFF_PATIENCE: git_diff_option_t = 1 << 28;
 pub const GIT_DIFF_MINIMAL: git_diff_option_t = 1 << 29;
 pub const GIT_DIFF_SHOW_BINARY: git_diff_option_t = 1 << 30;
-pub const GIT_DIFF_INDENT_HEURISTIC: git_diff_option_t = 1 << 31;
+pub const GIT_DIFF_INDENT_HEURISTIC: git_diff_option_t = 1 << 18;


Can you keep this numerically sorted with the others?

Sure thing. Done in 2538388

…tants

alexcrichton · 2019-05-23T14:02:53Z

git2-curl/src/lib.rs

@@ -75,6 +75,7 @@ pub unsafe fn register(handle: Easy) {
    let handle = Arc::new(Mutex::new(handle));
    let handle2 = handle.clone();
    INIT.call_once(move || {
+        libgit2_sys::init();


In general we try to auto-initialize libgit2 from the git2 wrapper crate, so could this call actually be moved into git2::transport::register? I think there's a top-level ::init() function as well in that crate

Moved libgit2_sys::init() into git2::transport::register. 4b4a1be

I played with moving it into places like: https://github.com/rust-lang/git2-rs/blob/master/src/lib.rs#L682, but no joy. Also not sure I understand how https://github.com/rust-lang/git2-rs/blob/master/src/lib.rs#L686 ties into:
https://github.com/rust-lang/git2-rs/blob/master/libgit2-sys/lib.rs#L3040
and: https://github.com/rust-lang/git2-rs/blob/master/libgit2-sys/lib.rs#L1594

alexcrichton · 2019-05-23T15:22:28Z

👍

ehuss · 2019-05-23T18:03:29Z

Ah, yea, this is definitely better, sorry for the confusion!

@bhamail Do you think you'll be able to look at further updating to a newer version from here? If not, I can take a look at the build issues.

bhamail · 2019-05-23T20:37:50Z

@ehuss Feel free to look into build errors with a newer version. I will try to take a look next week, if you haven't already done it. Thanks again for all the help.

bhamail · 2019-06-13T18:38:32Z

@ehuss I finally got back to trying to build against the latest libgit2. I was happy to find @cuviper beat me to it, and has already updated to the latest release from that project (#425). Thanks @cuviper.

update submodule libgit2 new commit to fix 'Host header omits non-def…

3d823d3

…ault port numbers' - rust-lang#405

bhamail mentioned this pull request May 17, 2019

Host header omits non-default port numbers #405

Closed

bhamail added 2 commits May 21, 2019 15:02

change constant GIT_DIFF_INDENT_HEURISTIC in lib.rs to match declarat…

764877b

…ion in libgit2-sys/libgit2/include/git2/diff.h Thanks @ehuss

attempt to fix seg fault in the all test. Thanks again @ehuss

d475dc9

alexcrichton reviewed May 22, 2019

View reviewed changes

ehuss reviewed May 22, 2019

View reviewed changes

bhamail added 3 commits May 22, 2019 11:45

reorder constant location to match sequential values of existing cons…

2538388

…tants

add version to dev dependency

437e22e

move init call into git2_curl::register

69334f8

alexcrichton reviewed May 23, 2019

View reviewed changes

move libgit2_sys::init() call to transport::register()

4b4a1be

alexcrichton merged commit ff12cad into rust-lang:master May 23, 2019

update submodule libgit2 to new commit to fix 'omits non-default port numbers' #421

update submodule libgit2 to new commit to fix 'omits non-default port numbers' #421

Uh oh!

Conversation

bhamail commented May 17, 2019

Uh oh!

bhamail commented May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ehuss commented May 18, 2019

Uh oh!

bhamail commented May 20, 2019

Uh oh!

ehuss commented May 21, 2019

Uh oh!

bhamail commented May 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhamail May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented May 23, 2019

Uh oh!

ehuss commented May 23, 2019

Uh oh!

bhamail commented May 23, 2019

Uh oh!

bhamail commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bhamail commented May 17, 2019 •

edited

Loading

bhamail May 22, 2019 •

edited

Loading

bhamail commented Jun 13, 2019 •

edited

Loading