Skip to content

Conversation

@hmelder
Copy link
Member

@hmelder hmelder commented Aug 11, 2022

There are still some patches required to the libs-base autoconf script to get the right version from pkg-config.

@hmelder hmelder marked this pull request as draft August 19, 2022 18:34
@hmelder
Copy link
Member Author

hmelder commented Aug 23, 2022

gnustep/libs-base#269

@triplef triplef force-pushed the enable-libcurl-support branch 6 times, most recently from 1af1267 to 551bde5 Compare December 22, 2022 13:35
@triplef triplef marked this pull request as ready for review December 22, 2022 13:39
@triplef
Copy link
Member

triplef commented Dec 22, 2022

Hi @hmelder, thanks for setting this up. After battling GitHub rate limit errors this is finally building on CI and seems to work ok.

I ran the tests locally and unfortunately there’s errors from these tests that we would probably still need to fix before merging this:

Running base/NSURLSession/test01.m...
Start set:       test01.m:79 ... NSURLSession test01
*   Trying 127.0.0.1:12345...
*   Trying [::1]:12345...
* Closing connection 0
Failed test:     (2022-12-22 14:42:41.072 +0100)   test01.m:125 ... request completed
Failed test:     (2022-12-22 14:42:41.072 +0100)   test01.m:127 ... unable to connect to host
2022-12-22 14:42:41.072 test01[15060:10120] Error (null)
End set:         test01.m:130 ... NSURLSession test01
Completed file:  test01.m

@hmelder
Copy link
Member Author

hmelder commented Dec 22, 2022

Hi @hmelder, thanks for setting this up. After battling GitHub rate limit errors this is finally building on CI and seems to work ok.

I ran the tests locally and unfortunately there’s errors from these tests that we would probably still need to fix before merging this:

Running base/NSURLSession/test01.m...
Start set:       test01.m:79 ... NSURLSession test01
*   Trying 127.0.0.1:12345...
*   Trying [::1]:12345...
* Closing connection 0
Failed test:     (2022-12-22 14:42:41.072 +0100)   test01.m:125 ... request completed
Failed test:     (2022-12-22 14:42:41.072 +0100)   test01.m:127 ... unable to connect to host
2022-12-22 14:42:41.072 test01[15060:10120] Error (null)
End set:         test01.m:130 ... NSURLSession test01
Completed file:  test01.m

Nice! I’ve seen the new commits. I’ll take a look at the errors in the unit tests, but it is probably related to the local web server.

@triplef
Copy link
Member

triplef commented Dec 22, 2022

That’d be fantastic, thanks. This test doesn’t have a local webserver, but instead tries to check if a connection to an nonexistent host fails correctly. I’m guessing it’s something with the libcurl integration.

@hmelder
Copy link
Member Author

hmelder commented Dec 28, 2022

That’d be fantastic, thanks. This test doesn’t have a local webserver, but instead tries to check if a connection to an nonexistent host fails correctly. I’m guessing it’s something with the libcurl integration.

That's correct. Internally GNUstep uses a curl multi handle that manages multiple easy handles. The handles itself are wrapped into objects, and there are a couple of quite complex handlers (Translating C-based callback functions into the proper NSURLSessionProtocol methods).

It would be great to have a CI log of the test. I guess I can just create a new branch in libs-base and modify the CI config?

@triplef
Copy link
Member

triplef commented Jan 3, 2023

I guess I can just create a new branch in libs-base and modify the CI config?

Yes, that’d be great! Thank you.

@triplef
Copy link
Member

triplef commented Jan 4, 2023

I updated libs-base to fix unreliable CI runs, and in the process added an option to easily specify a branch from this repo to use. Here’s a run I did with this libcurl branch:
https://github.com/gnustep/libs-base/actions/runs/3834875221/jobs/6527653656

@hmelder
Copy link
Member Author

hmelder commented Jan 8, 2023

I am just setting up a new x86 VM on a root server, but compilation of libdispatch fails:

I guess this is a problem in libdispatch upstream, as we are pulling the master branch.

image

@triplef
Copy link
Member

triplef commented Jan 8, 2023

Which Visual Studio version are you running? I'm guessing it's most likely related to that, as there haven't been any upstream changes in libdispatch in a while.

You'll ideally want to use VS
2019, as Clang in 2022 has a number of issues with ObjC.

@hmelder
Copy link
Member Author

hmelder commented Jan 8, 2023

You'll ideally want to use VS 2019, as Clang in 2022 has a number of issues with ObjC.

I am using VS 2022 with Windows Kit 11 and Clang 15.
I'll try it with VS 2019 and Clang 16 then.

@hmelder
Copy link
Member Author

hmelder commented Jan 8, 2023

MSVC supports atomic<> from version 2012 onwards (https://learn.microsoft.com/en-us/cpp/standard-library/atomic-enums?view=msvc-140). Weird.

@hmelder
Copy link
Member Author

hmelder commented Jan 8, 2023

I am using VS 2022 with Windows Kit 11 and Clang 15. I'll try it with VS 2019 and Clang 16 then.

Works with Clang 14

@hmelder
Copy link
Member Author

hmelder commented Jan 8, 2023

There is a minor difference between curl error handling on Windows and UNIX which results in different results when parsing them internally in urlErrorCodeWithEasyCode (https://github.com/gnustep/libs-base/blob/efccdb1d71166cfe096f309a97747175f46c1435/Source/GSEasyHandle.m#L271). I get -1001 (NSURLErrorTimedOut) on Windows and -1004 (NSURLErrorCannotConnectToHost) on Linux.

urlErrorCodeWithEasyCode uses the curl error and a system-specific error code to determine the corresponding Foundation Error Code CURLINFO_OS_ERRNO.

    else if (easyCode == CURLE_COULDNT_CONNECT && failureErrno == ETIMEDOUT) 
      {
        return NSURLErrorTimedOut;
      }

I'll look into this the next days. As I need to setup the dev vm and generate debug information.

image

image

@triplef triplef force-pushed the enable-libcurl-support branch from bdf9d1b to 39bb2fe Compare January 16, 2023 16:26
@triplef
Copy link
Member

triplef commented Jan 16, 2023

For some reason I actually don’t get any callback from curl when running the test locally, so the test’s run loop just times out...

I’ll try to look into it further since I’m working on various NSURLSession improvements.

@triplef triplef force-pushed the enable-libcurl-support branch from 0fec129 to 84a0c37 Compare January 20, 2023 10:11
@triplef triplef force-pushed the enable-libcurl-support branch 4 times, most recently from 6613a0f to 47abc4b Compare February 20, 2023 15:01
hmelder and others added 3 commits February 22, 2023 14:47
Fixes 32-bit builds, which were picking up libz.a and libssh2.a found in this directory.

More information:
actions/runner-images#6627
@triplef triplef force-pushed the enable-libcurl-support branch from 47abc4b to 498a258 Compare February 22, 2023 13:48
@triplef triplef merged commit 9822803 into master Feb 22, 2023
@triplef triplef deleted the enable-libcurl-support branch February 22, 2023 13:48
@triplef
Copy link
Member

triplef commented Feb 22, 2023

Unfortunately I didn’t manage to track down the NSURLSession/test01 failure, so for now I marked it as hopeful as part of my NSURLSession improvements PR gnustep/libs-base#286.

With that the libs-base CI passes:
https://github.com/gnustep/libs-base/actions/runs/4225184793

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants