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

Use .curlrc and .netrc windows as well #4230

Closed
wants to merge 2 commits into from
Closed

Conversation

bagder
Copy link
Member

@bagder bagder commented Aug 16, 2019

  1. Make libcurl on windows first check for .netrc then if missing, go for _netrc
  2. Make curl on windows first check for .curlrc then if missing, go for .curlrc

This is my refresh of the code started in #3989 by @captain-caveman2k. The main point of this exercise is unification. Stick to the dot version primarily, support the underscore for compatibility.

The _netrc thing is marked as a "known issue" for git on windows.

I'm thinking maybe @dscho, @jay and @gvanem might have opinions.

@bagder
Copy link
Member Author

bagder commented Aug 16, 2019

I have not built or tested this on Windows, only written the code to hopefully do roughly the right thing! 😀

@dscho
Copy link
Contributor

dscho commented Aug 16, 2019

Looks good, after a preliminary review. I did not get a chance yet to test this properly, hopefully by the end of next week.

@bagder
Copy link
Member Author

bagder commented Aug 16, 2019

The red appveyor builds puzzle me!

@dscho
Copy link
Contributor

dscho commented Aug 17, 2019

The red appveyor builds puzzle me!

Indeed. It seems that ../src/curl.exe --version segfaults... :-(

@bagder
Copy link
Member Author

bagder commented Aug 17, 2019

I presume then that something in this new curlrc code of mine is totally borked...

@jay
Copy link
Member

jay commented Aug 19, 2019

I can't make it crash with --version but I don't know how to build it the same exact way as appveyor. VS in debug mode doesn't detect anything. I chose rebuild pr I'm curious if it's arbitrary or the results will be the same.

@gvanem
Copy link
Contributor

gvanem commented Aug 19, 2019

@jay I can if I do:

set APPDATA=
set HOME=
curl.exe --version

The WinDbg call-stack points the finger at:

ucrtbased!strcmp(unsigned char * str1 = 0x00000000 "", unsigned char * str2 = 0x005bf900 "???")+0x10 [minkernel\crts\ucrt\src\appcrt\string\i386\strcmp.asm @ 82] 
curl!parseconfig(char * filename = 0x00000000 "", struct GlobalConfig * global = 0x005bf900)+0x1b1 [F:\MingW32\src\inet\curl\src\tool_parsecfg.c @ 133] 

Unusual I know to have neither of these. Something that should be considered?

@bagder
Copy link
Member Author

bagder commented Aug 19, 2019

Ah, that remark made me realize the new logic only checks for curlrc in the excutable's directory if it figures out a home directory, which seems wrong! I'll fix.

... but fall back and try "_netrc" too if the dot version didn't work.

Co-Authored-By: Steve Holme
@bagder
Copy link
Member Author

bagder commented Aug 19, 2019

edited, squashed and rebased

@bagder
Copy link
Member Author

bagder commented Aug 19, 2019

Oops, torture test failures. This is leaking memory in some exit paths. I'll fix...

Fall-back to _curlrc if the dot-version is missing.

Co-Authored-By: Steve Holme
@bagder bagder closed this in 8623932 Aug 20, 2019
@bagder bagder deleted the bagder/curlrc-windows branch August 20, 2019 09:50
@gvanem
Copy link
Contributor

gvanem commented Aug 20, 2019

Considering the message in url.c:

      infof(data, "Couldn't find host %s in the "
            DOT_CHAR "netrc file; using defaults\n",
            conn->host.name);

Shouldn't curl_setup.h be patched too?

--- a/curl_setup.h 2019-08-04 11:16:15
+++ b/curl_setup.h 2019-08-20 10:48:34
@@ -486,7 +486,7 @@
 #ifdef WIN32

 #  define DIR_CHAR      "\\"
-#  define DOT_CHAR      "_"
+#  define DOT_CHAR      "."

 #else /* WIN32 */

bagder added a commit that referenced this pull request Aug 20, 2019
Follow-up to f9c7ba9

The use of DOT_CHAR for ".ssh" was probably a mistake and is removed
now.

Pointed-out-by: Gisle Vanem
Bug: #4230 (comment)
bagder added a commit that referenced this pull request Aug 20, 2019
Follow-up to f9c7ba9

The use of DOT_CHAR for ".ssh" was probably a mistake and is removed
now.

Pointed-out-by: Gisle Vanem
Bug: #4230 (comment)

Closes #4247
@lock lock bot locked as resolved and limited conversation to collaborators Nov 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants