-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add support for builtin libcurl and some other fixes #4
Conversation
Cloudef
commented
Dec 21, 2023
- Fix crash on easy.do() which does not pass the proper type to curl
- Fix compile on latest zig
- Use standard optimize options in built.zig
- Fix typo in errors.zig
- Allow the use of certificates in std.crypto.Certificate.Bundle instead of default one
- Include builtin libcurl compiled against mbedTls, the use of std.crypto.Certificate.Bundle is required, or CURLOPT_CAINFO should be used to point to cacert bundle.
Libcurl expects pointer to a long, this fixes a crash.
Note that the zigcurl dependency points to my fork and should be updated once star-tek-mb/zigcurl#2 is merged. |
Side-note, it would be nice to have std.http.Client's fetch compatible api. I use this in my own project until std.crypto has TLSv1.2 support: // Temporary crap until std.http.Client has TLSv1.2 support
// https://github.com/ziglang/zig/issues/14172#issuecomment-1384567705
const curl = @import("curl");
const Client = struct {
client: curl.Easy,
pub fn init(allocator: std.mem.Allocator) !@This() {
return .{ .client = try curl.Easy.init(allocator) };
}
pub fn deinit(self: *@This()) void {
self.client.deinit();
}
pub fn fetch(self: *@This(), allocator: std.mem.Allocator, options: std.http.Client.FetchOptions) !std.http.Client.FetchResult {
var reader = switch (options.payload) {
.file => |file| file.reader().any(),
.string => |str| blk: {
var stream = std.io.fixedBufferStream(str);
break :blk stream.reader().any();
},
.none => blk: {
const empty: []const u8 = &.{};
var stream = std.io.fixedBufferStream(empty);
break :blk stream.reader().any();
},
};
const target = switch (options.location) {
.url => |url| url,
.uri => @panic("unsupported"),
};
var headers = curl.Easy.RequestHeader.init(allocator);
for (options.headers.list.items[0..]) |header| try headers.add(header.name, header.value);
var req = curl.Easy.Request(*@TypeOf(reader)).init(target, &reader, .{
.method = std.meta.stringToEnum(curl.Easy.Method, @tagName(options.method)).?,
.header = headers,
});
defer req.deinit();
const res = try self.client.do(req);
return .{
.allocator = allocator,
.options = options,
.status = @enumFromInt(res.status_code),
.body = res.body.items,
.headers = .{ .allocator = allocator },
};
}
}; |
This is a nice point, we can do this in another PR. |
The dependency points to a fork. Should be updated to upstream once star-tek-mb/zigcurl#2 is merged!
The Lines 233 to 263 in 96ba813
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I will do some polish based on your branch, please don't update any more. |
@jiacai2050 since there's no activity on the libcurl PR it might be better merge into this project. I also fixed ARM compilation Cloudef/zigcurl@e1266d8 |
I do not like to copy all c files directly into this project, it's hard to update and we don't know the difference between ours and upstream. Could we use git submodule to add those C packages? |
I think it's possible. In the meantime star-tek-mb/zigcurl#2 the PR with fixes was merged in. |