search

libgit2 / pygit2

Python bindings for libgit2
https://www.pygit2.org/
Other
1.59k stars 383 forks source link

A new version please? (1.6.0 or something) #1069

Closed feisuzhu closed 3 years ago

feisuzhu commented 3 years ago

We are expecting the proxy feature and it seems landed on master (https://github.com/libgit2/pygit2/pull/1063). We have been waiting for a long time.

jdavid commented 3 years ago

Can you test the wheels https://github.com/libgit2/pygit2/actions/runs/711340943 ?

feisuzhu commented 3 years ago

2021-04-06-162103_1097x487_scrot

Probably not……

feisuzhu commented 3 years ago

Additionally, after fixing the offending line

proxy_opts.url = ffi.new('char[]', to_bytes(proxy))

things become weird

2021-04-06-175038_1265x721_scrot

Should be localhost not localhostP

It's not 100% reproducible, but definitely would show up after times.

jdavid commented 3 years ago

Do you think you could make a PR with a unit test showing the error?

jdavid commented 3 years ago

ping @sathieu

sathieu commented 3 years ago

I'm just using the AUTO mode (proxy=True) together with https://github.com/libgit2/libgit2/pull/5796, this works perfectly. Need to dig in ffi to understand the problem.

sathieu commented 3 years ago

@feisuzhu Have you tried:

proxy_opts.url = ffi.string(proxy)

Ref: https://cffi.readthedocs.io/en/latest/ref.html#ffi-string-ffi-unpack

sathieu commented 3 years ago

@feisuzhu Have you tried:

proxy_opts.url = ffi.string(proxy)

Ref: https://cffi.readthedocs.io/en/latest/ref.html#ffi-string-ffi-unpack

No, no, no. Sorry for the noise ...

feisuzhu commented 3 years ago

I'm just using the AUTO mode (proxy=True) together with libgit2/libgit2#5796, this works perfectly. Need to dig in ffi to understand the problem.

Can confirm, I end up using proxy=True too.

sathieu commented 3 years ago

@feisuzhu Have you tried:

proxy_opts.url = ffi.new('char[]', proxy)

ffi has a doc about memory ownership.

Otherwise @jdavid, maybe we should drop support for proxy=url?

feisuzhu commented 3 years ago

@feisuzhu Have you tried:

proxy_opts.url = ffi.new('char[]', proxy)

ffi has a doc about memory ownership.

Otherwise @jdavid, maybe we should drop support for proxy=url?

IMHO ffi.new('char[]', to_bytes(proxy)) returns an owned reference(to_bytes returns Python bytes, not another cdata), so the problem lies elsewhere.

I'll try again though.

sathieu commented 3 years ago

@feisuzhu Yes but the result of to_bytes(proxy) is released as soon a this line is done.

feisuzhu commented 3 years ago

@sathieu

Did some experiment, found that it's ffi.new('char[]', xxx) should be kept referenced, not the bytes. If I keep the char[] referenced in global the issue disappears.

jdavid commented 3 years ago

Pushed a commit. Can you try with the wheels at https://github.com/libgit2/pygit2/actions/runs/774665500#artifacts ?