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

Fixes for --disable-doh #4692

Closed
wants to merge 3 commits into from
Closed

Conversation

MarcelRaad
Copy link
Member

This fixes the build and tests for --disable-doh --disable-threaded-resolver --with-nghttp2.

@MarcelRaad
Copy link
Member Author

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.

@jay
Copy link
Member

jay commented Dec 9, 2019

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:

crlf_to_lf

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.

@jay
Copy link
Member

jay commented Dec 9, 2019

also why did you remove the <name> tags from the other two

edit: hm... seems to be gitk. I think git diff is just nutty with these mixed line ending files. In both cases it was and is <name>\n so there's no actual change. Weird.

incorrect diff in gitk

@MarcelRaad
Copy link
Member Author

MarcelRaad commented Dec 9, 2019

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.

also why did you remove the tags from the other two

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 :-) )

@jay
Copy link
Member

jay commented Dec 9, 2019

Uhm, what did I do? That was not on purpose and I see that neither in my local diff nor on Github.

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.

@MarcelRaad
Copy link
Member Author

Hm, test 2100 still fails in the AppVeyor MSYS autotools build because of:

curl: (6) Could not DOH-resolve: (nil)

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.

@bagder
Copy link
Member

bagder commented Dec 10, 2019

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...

@MarcelRaad
Copy link
Member Author

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.

./configure --enable-debug --enable-werror
[...]
test 2100...[HTTP GET using DOH]
--pd---e--- OK (1   out of 1  , remaining: 00:00, took 2.4s, duration: 00:02)
TESTDONE: 1 tests out of 1 reported OK: 100%
TESTDONE: 1 tests were considered during 2 seconds.
./configure --enable-debug --enable-werror --without-ssl --without-zlib --disable-threaded-resolver --enable-alt-svc --disable-proxy
[...]
test 2100...[HTTP GET using DOH]

 2100: protocol FAILED:
Binary files log/check-expected and log/check-generated differ

 - abort tests
TESTDONE: 0 tests out of 1 reported OK: 0%
TESTFAIL: These test cases failed: 2100
TESTDONE: 1 tests were considered during 3 seconds.

@bagder
Copy link
Member

bagder commented Dec 11, 2019

I figured it out! Stand by for separate PR to fix that issue.

bagder added a commit that referenced this pull request Dec 11, 2019
bagder added a commit that referenced this pull request Dec 12, 2019
@bagder
Copy link
Member

bagder commented Dec 12, 2019

#4704 landed now so you should probably rebase this and try again!

MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Dec 12, 2019
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Dec 12, 2019
With `--disable-doh --disable-threaded-resolver`, the `dns` parameter
is not used.

Closes curl#4692
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Dec 12, 2019
Previously, http/2 was used instead.

Assisted-by: Jay Satiro
Closes curl#4692
@MarcelRaad
Copy link
Member Author

MarcelRaad commented Dec 12, 2019

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 test* -crlf.

=== Start of file check-expected
 POST /21000001 HTTP/1.1[LF]
 Host: 127.0.0.1:8990[LF]
 Accept: */*[LF]
 Content-Type: application/dns-message[LF]
 Content-Length: 33[LF]
 [LF]
 �������������foo�example�com�����GET /2100 HTTP/1.1[LF]
 Host: foo.example.com:8990[LF]
 Accept: */*[LF]
 [LF]
=== End of file check-expected
=== Start of file check-generated
 POST /21000001 HTTP/1.1[CR][LF]
 Host: 127.0.0.1:8990[CR][LF]
 Accept: */*[CR][LF]
 Content-Type: application/dns-message[CR][LF]
 Content-Length: 33[CR][LF]
 [CR][LF]
 �������������foo�example�com�����GET /2100 HTTP/1.1[CR][LF]
 Host: foo.example.com:8990[CR][LF]
 Accept: */*[CR][LF]
 [CR][LF]
=== End of file check-generated

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
@MarcelRaad
Copy link
Member Author

Now maybe?

@MarcelRaad
Copy link
Member Author

Failed to start: The resource 'projects/freebsd-org-cloud-dev/global/images/family/freebsd-10-4' was not found

Looks unrelated, as do the test1501 failures on AppVeyor.

@jay
Copy link
Member

jay commented Dec 12, 2019

In your most recent commit d1d19f3, test2100 is a byte for byte match except the change to DoH.

@bagder
Copy link
Member

bagder commented Dec 12, 2019

1501 is a very common flaky/false positive test failure on appveyor. I kicked off a rerun of the failed jobs just now.

@bagder
Copy link
Member

bagder commented Dec 13, 2019

The msys build failure seems to indicate CRLF / line ending problems in some tests but I can't say I understand that...

@dfandrich
Copy link
Contributor

dfandrich commented Dec 13, 2019 via email

@MarcelRaad
Copy link
Member Author

@bagder

The msys build failure seems to indicate CRLF / line ending problems in some tests but I can't say I understand that...

Most of them seem to be because of

curl: (7) Failed to connect to 127.0.0.1 port 9013: Connection refused

I haven't seen any line ending differences. Do you have any specific test in mind?

@bagder
Copy link
Member

bagder commented Dec 13, 2019

Ah, no I must have I misread. They are indeed that connection refused problems

MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Dec 13, 2019
With `--disable-doh --disable-threaded-resolver`, the `dns` parameter
is not used.

Closes curl#4692
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Dec 13, 2019
Previously, http/2 was used instead.

Assisted-by: Jay Satiro
Closes curl#4692
@MarcelRaad MarcelRaad deleted the tests_disable_doh branch December 13, 2019 19:56
@MarcelRaad
Copy link
Member Author

I'm confident that this version works now and all of the test failures were unrelated. Thanks a lot for your help @jay and @bagder !

@lock lock bot locked as resolved and limited conversation to collaborators Mar 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

4 participants