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

Fix/library not supporting curl version 8.0.x or higher, Closes #10 #85

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

Conversation

Laeri
Copy link

@Laeri Laeri commented Mar 22, 2023

Closes #84

Laeri added 2 commits March 22, 2023 11:37
Modify `misc/compatgen.py` to emit compatability also with
version `8.0.0` or higher.
@Laeri Laeri marked this pull request as draft March 22, 2023 04:31
@Laeri
Copy link
Author

Laeri commented Mar 22, 2023

Some of the errors disappeared, but I still have the following ones with my changes.

• Generating bindings: 
  ERROR   
          # github.com/Laeri/go-curl
          ../../../go/pkg/mod/github.com/!laeri/[email protected]/const_gen.go:148:34: could not determine kind of name for C.CURLOPT_CLOSEFUNCTION
          ../../../go/pkg/mod/github.com/!laeri/[email protected]/const_gen.go:127:34: could not determine kind of name for C.CURLOPT_FTPASCII
          ../../../go/pkg/mod/github.com/!laeri/[email protected]/const_gen.go:111:34: could not determine kind of name for C.CURLOPT_HTTPREQUEST
          ../../../go/pkg/mod/github.com/!laeri/[email protected]/const_gen.go:130:34: could not determine kind of name for C.CURLOPT_MUTE
          ../../../go/pkg/mod/github.com/!laeri/[email protected]/const_gen.go:78:34: could not determine kind of name for C.CURLOPT_NOTHING
          ../../../go/pkg/mod/github.com/!laeri/[email protected]/const_gen.go:142:34: could not determine kind of name for C.CURLOPT_PASSWDDATA
          ../../../go/pkg/mod/github.com/!laeri/[email protected]/const_gen.go:141:34: could not determine kind of name for C.CURLOPT_PASSWDFUNCTION

@Laeri
Copy link
Author

Laeri commented Mar 22, 2023

Ok I found out that these options that result in an error when compiling were removed going from version 7.15.5 to 7.16.0.
I am not sure how this works as compatgen should only have checked for minor version 10 or higher.

Do you think that simply requiring a minor version of 16 or higher is enough to finish this pull request?

ilmari-lauhakangas added a commit to ilmari-lauhakangas/go-curl that referenced this pull request Apr 5, 2023
@ilmari-lauhakangas
Copy link

I got help on #go-nuts in Libera Chat to make this patch work. I was building a program that uses go-curl as a dependency (cameradar) and it used constants that this patch removes. I worked around the problem by adding them back into const_gen.go like so:

OPT_RTSP_STREAM_URI           = C.CURLOPT_RTSP_STREAM_URI
OPT_RTSP_REQUEST              = C.CURLOPT_RTSP_REQUEST
OPT_RTSP_TRANSPORT            = C.CURLOPT_RTSP_TRANSPORT
OPT_TIMEOUT_MS                = C.CURLOPT_TIMEOUT_MS

But the fundamental issue diagnosis I got on #go-nuts was:

It's looking specifically for any lines with a #define CURLOPT_ prefix, but looking at current curl sources, those options have been changed to be enum constants now.

So codegen.py would have to be updated to support the new format.

@Laeri
Copy link
Author

Laeri commented Apr 18, 2023

@ilmari-lauhakangas Thanks for having a look at it. I am currently working on other things so I won't be able to do your suggested changes for your info. Hopefully you or someone else can implement the missing changes.

@batmac
Copy link

batmac commented Apr 18, 2023

Hi, If it can help in the meantime, I've fixed my go-curl usage with this trivial change: master...batmac:go-curl:batmac

@sprive
Copy link
Contributor

sprive commented Jul 3, 2024

Hello @andelf. Thank you for go-curl. Would you be able to review some pending PRs which fix blockers? Thank you.

@andelf
Copy link
Owner

andelf commented Jul 3, 2024

@sprive Apologies, it's been quite some time since I've worked with Go. Since Go uses a repo-URL based package management system, I haven't given much attention to maintaining this particular repository.

I noticed this PR is still a working-in-progress. Could you provide some hints on which PR I should merge?

This project starts at the year 2011. It was the first Go code that implemented a bidirectional callback at that time.

@sprive
Copy link
Contributor

sprive commented Jul 5, 2024

Hello @andelf Definitely do not apologize (this looks great and I see you are exceptionally busy on other projects/languages). I am grateful you put this together as I have some use cases that are best handled by libcurl. :-)

So... I had the same error as @Laeri - building go-curl on a system with libcurl 8 or higher. I solved my problem more simply by adding LIBCURL_VERSION_MAJOR == 7:

compa:t.h::

#if (LIBCURL_VERSION_MAJOR == 7 && LIBCURL_VERSION_MINOR == 10 && LIBCURL_VERSION_PATCH < 1) || (LIBCURL_VERSION_MAJOR == 7 && LIBCURL_VERSION_MINOR < 10)
#error your version is TOOOOOOOO low

^^ fixed my build failure. :party:

What I could do (please advise):
option a) I pr only the above?
option b) I extend the LIBCURL_VERSION_MAJOR == 7 (blindly) to every single check in compat.h?
This type of change (and done without testing) gives me pause, if I don't a +1 first.

I would create a standalone PR for whichever you choose.

Unfortunately I'm not into libcurl (or C) enough to validate what all the different defines are for. But since these rules were written pre-curl 8, it seems to me to be OK in assuming they all mean LIBCURL_VERSION_MAJOR == 7.

@andelf
Copy link
Owner

andelf commented Jul 5, 2024

@sprive Feel free to add a PR.😁
I remember that compat.h is generated using a Python script, filling all missing symbols.

@sprive sprive mentioned this pull request Jul 6, 2024
@sprive
Copy link
Contributor

sprive commented Jul 6, 2024

Related: #88

PR#88 has some of the cleanup contained in here (but does not try to address the main curl8 issue, yet)

@sprive
Copy link
Contributor

sprive commented Jul 16, 2024

Related: #89

PR#89 contains the remainder of things not in the first PR above.
(if 89 is accepted, then "this" PR can be closed. Or I will remind to do so afterwards)

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

Successfully merging this pull request may close these issues.

library not supporting curl version 8.0.X or higher
5 participants