-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Fixes for --disable-doh
#4692
Fixes for --disable-doh
#4692
Conversation
a1a7f19
to
2cda9ce
Compare
Could someone else with a non-Windows machine please replace the http/2 in test2100 with DoH? I just can't get a working test2100 on my Windows machine. Trying to edit it with native Windows tools and with nano on WSL lead to differently broken test2100 files. |
2cda9ce
to
36bc123
Compare
It looks like your editor changed any CRLF into LF, and most tests contain both. Here's an animated GIF of Notepad++ w/ Show Symbol > Show End-of-line that switches back and forth from the previous version so you can see what I mean: It's treated as a binary file which makes it hard to diff, and git diff --text doesn't work properly on it, it will say areas are different when they are not just because it doesn't recognize the CRs. I did it with a clean copy of test2100 in Notepad++, changed it to DoH, amended that test2100 to your last commit and force pushed it. |
Great, thanks a lot Jay! Also, good to know that Notepad++ works. nano was the one converting CRLF to LF, Visual Studio refused to open the file, and Notepad and TortoiseGit Diff seem to have broken it in other ways.
Uhm, what did I do? That was not on purpose and I see that neither in my local diff nor on Github. (Edit: I've just seen your edit :-) ) |
Yeah it's gitk I updated my comment to strike that and include a screenshot of what I see here. I double checked in a hex editor and there is no change there. |
Hm, test 2100 still fails in the AppVeyor MSYS autotools build because of:
This test wasn't run before because of lacking HTTP/2 support. 1650 and 1655 succeed though and I haven't noticed anything HTTP/2-specific in test 2100. Trying to reproduce locally that now. |
Curious. I tried doing a local build on Linux without HTTP/2 and the stock synchronous resolver on your branch, but it doesn't repro the failure... |
Very strange. I could reproduce this now with MSYS, but also without my commit, so the relevant change is that test 2100 gets executed now at all.
|
I figured it out! Stand by for separate PR to fix that issue. |
Reported-by: Marcel Raad Bug: #4692 (comment)
Reported-by: Marcel Raad Bug: #4692 (comment) Closes #4704
#4704 landed now so you should probably rebase this and try again! |
36bc123
to
2c15cf8
Compare
These test 2100 failures are new. Did I manage to break the line endings by doing nothing but rebase now? According to .gitattributes, there should be no line ending coversion done as it specifies
|
2c15cf8
to
d1d19f3
Compare
Now maybe? |
Looks unrelated, as do the test1501 failures on AppVeyor. |
In your most recent commit d1d19f3, test2100 is a byte for byte match except the change to DoH. |
1501 is a very common flaky/false positive test failure on appveyor. I kicked off a rerun of the failed jobs just now. |
The msys build failure seems to indicate CRLF / line ending problems in some tests but I can't say I understand that... |
Failed to start: The resource 'projects/freebsd-org-cloud-dev/global/images
/family/freebsd-10-4' was not found
Upstream support for FreeBSD 10.4 ended a year ago, so I'm not too surprised to
see it disappearing from Cirrus. I'll submit a change to drop it.
|
Most of them seem to be because of
I haven't seen any line ending differences. Do you have any specific test in mind? |
Ah, no I must have I misread. They are indeed that |
With `--disable-doh --disable-threaded-resolver`, the `dns` parameter is not used. Closes curl#4692
Previously, http/2 was used instead. Assisted-by: Jay Satiro Closes curl#4692
This fixes the build and tests for
--disable-doh --disable-threaded-resolver --with-nghttp2
.