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

Conversation

@bagder
Copy link
Member

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 bagder force-pushed the bagder/curlrc-windows branch from feb9978 to 4a0e44c Aug 16, 2019

@bagder

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

The red appveyor builds puzzle me!

@dscho

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

The red appveyor builds puzzle me!

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

@bagder

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

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

@jay

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

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.

netrc: make the code try ".netrc" on Windows as well
... but fall back and try "_netrc" too if the dot version didn't work.

Co-Authored-By: Steve Holme

@bagder bagder force-pushed the bagder/curlrc-windows branch from fb05dcc to d0757e2 Aug 19, 2019

@bagder

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

edited, squashed and rebased

@bagder

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

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

curl: use .curlrc (with a dot) on Windows as well
Fall-back to _curlrc if the dot-version is missing.

Co-Authored-By: Steve Holme

@bagder bagder force-pushed the bagder/curlrc-windows branch from b560365 to 6696d45 Aug 20, 2019

@bagder bagder closed this in 8623932 Aug 20, 2019

@bagder bagder deleted the bagder/curlrc-windows branch Aug 20, 2019

@gvanem

This comment has been minimized.

Copy link
Member

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
cleanup: remove DOT_CHAR completely
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
cleanup: remove DOT_CHAR completely
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.