Skip to content
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

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

Closed
feisuzhu opened this issue Mar 30, 2021 · 14 comments
Closed

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

feisuzhu opened this issue Mar 30, 2021 · 14 comments

Comments

@feisuzhu
Copy link

We are expecting the proxy feature and it seems landed on master (#1063).
We have been waiting for a long time.

@jdavid
Copy link
Member

jdavid commented Apr 2, 2021

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

@feisuzhu
Copy link
Author

feisuzhu commented Apr 6, 2021

2021-04-06-162103_1097x487_scrot

Probably not……

@feisuzhu
Copy link
Author

feisuzhu commented Apr 6, 2021

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
Copy link
Member

jdavid commented Apr 9, 2021

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

@jdavid
Copy link
Member

jdavid commented Apr 9, 2021

ping @sathieu

@sathieu
Copy link
Contributor

sathieu commented Apr 9, 2021

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.

@sathieu
Copy link
Contributor

sathieu commented Apr 9, 2021

@feisuzhu Have you tried:

proxy_opts.url = ffi.string(proxy)

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

@sathieu
Copy link
Contributor

sathieu commented Apr 9, 2021

@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
Copy link
Author

feisuzhu commented Apr 9, 2021

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
Copy link
Contributor

sathieu commented Apr 9, 2021

@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
Copy link
Author

@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
Copy link
Contributor

sathieu commented Apr 11, 2021

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

@feisuzhu
Copy link
Author

@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 added a commit that referenced this issue Apr 22, 2021
Issue #1069

Test skipped as it triggers a bug in libgit2 causing a segfault. Just
before this message is printed:

src/transports/httpclient.c:926: proxy_connect: Assertion `client->state == DONE' failed.
@jdavid
Copy link
Member

jdavid commented Apr 22, 2021

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

@jdavid jdavid closed this as completed Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants