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

Compatibility with LibCURL's new generator script #134

Closed
wants to merge 1 commit into from

Conversation

melonedo
Copy link

Ref: LibCURL.jl#102
I have tested this new wrapper, and Downloads.jl does not work with the new wrapper, mainly because:

  • Enum types were mapped directly to integers in LibCURLjl, and hardcoded to Integer in Downloads.jl
  • LibCURL.jl's constants are out of sync

this patch adds compatibility to the new wrapper. Most tests are passed, except the ones that can not be run on my network (curl https://httpbingo.julialang.org/base64/SnVsaWEgaXMgZ3JlYXQh returns curl: (35) OpenSSL SSL_connect: Connection reset by peer in connection to httpbingo.julialang.org:443 without a proxy, for example)

@melonedo
Copy link
Author

Oh this commit uses a custom fork https://github.com/melonedo/LibCURL.jl.git#up-generator. Sorry but I do not know how to run CI on this, I changed the UUID to tested it locally.

src/Downloads.jl Outdated Show resolved Hide resolved
src/Curl/Curl.jl Outdated Show resolved Hide resolved
@@ -33,7 +33,7 @@ function check(ex::Expr, lock::Bool)
end
quote
r = $ex
iszero(r) || @async @error($prefix * string(r))
iszero(Integer(r)) || @async @error($prefix * string(r))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems incorrect. What's the intention here? Integer is an abstract type, not a concrete one. This won't change the type of any integer value, it will only affect floating-point values, and even if one somehow got a float here, iszero would still work.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The appropriate fix is to add another definition: Base.zero(::CEnum.Cenum{T}) where {T<:Integer} = zero(T).

Project.toml Outdated Show resolved Hide resolved
@melonedo
Copy link
Author

Sorry I did not know the design of Downloads.jl, I thought the code was only intended to work with LibCURL.jl. If so, it should directly map to LibCURL's constants instead of some integer values. For example, the @checked macro tests status code against zero, which is not very well defined for enumerations, so I convert it to integer before comparing it.
But since Downloads.jl is platform independent, I think this can be fixed on LibCURL.jl.

@melonedo melonedo closed this Jul 23, 2021
@StefanKarpinski
Copy link
Member

Thanks for the effort here. A few general comments. Integer(x) doesn't convert to a specific integer type — Integer is the abstract supertype of all integer types, so Integer(x) just converts a value to some kind of integer, not any specific kind. Example:

julia> Integer(1)
1

julia> Integer(0x1)
0x01

julia> Integer(1.0)
1

Those LibCURL constants are all already integers so doing Integer(x) on them has no effect:

julia> LibCURL.CURLE_PEER_FAILED_VERIFICATION
0x00000033

julia> typeof(ans)
UInt32

julia> Integer(LibCURL.CURLE_PEER_FAILED_VERIFICATION)
0x00000033

So, for example when you write Integer(LibCURL.CURLE_PEER_FAILED_VERIFICATION) != 60 there's no point in that Integer call since it doesn't do anything. It would probably make sense to have a conditional here like the second one you added to check if CURLE_PEER_FAILED_VERIFICATION is defined. There's no point in checking if its value is 60 or not and then defining it as 60 if it isn't. That's logically equivalent to just defining it as 60. So the pattern here for constants that were exported by some versions of LibCURL and not by others would be to check if they're defined and if not, define them. That way if, in the future, some version of LibCURL wraps a different version of libcurl that defines these differently, it will work.

The CURLoption type is just a platform-specific alias for some integer type. On my system, for example CURLoption is UInt32 which is why all the curl option values have type UInt32. When you change the signature of a function like setopt from setopt(easy::Easy, option::Integer, value) to setopt(easy::Easy, option::CURLoption, value) you are restricting how it can be called: now it can only be called with a UInt32 value specifically, whereas before it could be called with any kind of integer value. Since the ccall stubs that Clang.jl generates convert values to the type that the C function expects, it's fine for setopt to accept any kind of integer because it will get converted to CURLoption when the libcurl ccall stub is called.

The fields in the RequestError struct intentionally do not reflect the CURLcode type. I don't want the layout and behavior of this struct to depend on what the C compiler happens to chose for the CURLcode type. The Int type is big enough to hold all the values of CURLcode that actually are used. When constructing a RequestError object, the error code of type CURLcode will be automatically converted to the type of the code field, so this is not a problem.

src/Curl/Curl.jl Outdated Show resolved Hide resolved
@@ -33,7 +33,7 @@ function check(ex::Expr, lock::Bool)
end
quote
r = $ex
iszero(r) || @async @error($prefix * string(r))
iszero(Integer(r)) || @async @error($prefix * string(r))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The appropriate fix is to add another definition: Base.zero(::CEnum.Cenum{T}) where {T<:Integer} = zero(T).

@Gnimuc
Copy link

Gnimuc commented Jul 24, 2021

@melonedo would you like to reopen this and adapt to the comments?

@melonedo melonedo reopened this Jul 25, 2021
@melonedo
Copy link
Author

melonedo commented Aug 7, 2021

Now I included conversion to Int everywhere it needs to, tests are all passed (with new LibCURL).

@melonedo
Copy link
Author

Closed in favor of JuliaWeb/LibCURL.jl#105

@melonedo melonedo closed this Aug 24, 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

Successfully merging this pull request may close these issues.

3 participants