Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix environment on Windows #6

Merged
merged 1 commit into from
Mar 7, 2022
Merged

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Mar 15, 2021

On Windows, an empty environment can cause curl to fail.

In particular, running curl seems to require PATH and SYSTEMROOT:

  • if PATH is not set, the binary is not found
  • if SYSTEMROOT is not set, address resolution fails at runtime

@emillon
Copy link
Collaborator Author

emillon commented Mar 15, 2021

Ping @dra27 - do you know what would cause a cygwin binary started from a non-cygwin binary (everyone's favorite combination) to start correctly, but having no network access? I found that passing PATH and SYSTEMROOT fixes the issue but I'm not completely sure why this is working.

@dra27
Copy link

dra27 commented Mar 15, 2021

It's nothing to do with Cygwin...

C:\>set SYSTEMROOT=

C:\>C:\Windows\System32\curl.exe https://github.com
curl: (6) Could not resolve host: github.com

C:\>set SYSTEMROOT=C:\Windows

C:\>C:\Windows\System32\curl.exe -O https://github.com
curl: Remote file name has no length!
curl: try 'curl --help' for more information

C:\>C:\Windows\System32\curl.exe -LO https://github.com
curl: Remote file name has no length!
curl: try 'curl --help' for more information

C:\>C:\Windows\System32\curl.exe -o index.html https://github.com
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current

@dra27
Copy link

dra27 commented Mar 15, 2021

I just quickly checked - getaddrinfo definitely fails if SYSTEMROOT isn't set. I haven't dug into precisely why, but it doesn't surprise me - it's probably trying to load %SYSTEMROOT%\system32\drivers\etc\hosts...

On Windows, an empty environment can cause curl to fail.
In particular, running curl seems to require `PATH` and `SYSTEMROOT`:

- if `PATH` is not set, the binary is not found
- if `SYSTEMROOT` is not set, address resolution fails at runtime
@emillon
Copy link
Collaborator Author

emillon commented Mar 15, 2021

Ah! I was too quick to blame poor cygwin. I updated the description. Thanks!

@rgrinberg
Copy link
Owner

It seems strange that we are running the process with an empty environment in the first place. Can we just inherit the environment? I think that's what users would expect.

@dra27
Copy link

dra27 commented Mar 15, 2021

Depends on how thin a wrapper you're after - there are environment variables which can control curl which presently aren't exposed to it

@emillon
Copy link
Collaborator Author

emillon commented Mar 16, 2021

Yes, including *_PROXY, CURL_CA_BUNDLE, SSLKEYLOGFILE... The curl process is shielded from these at the moment. Most of these can be passed as an argument so I think it's ok to clear them (but it's a design decision indeed).

@rgrinberg
Copy link
Owner

Depends on how thin a wrapper you're after - there are environment variables which can control curl which presently aren't exposed to it

Would these environment variables control libcurl as well?

@dra27
Copy link

dra27 commented Mar 17, 2021

Depends on how thin a wrapper you're after - there are environment variables which can control curl which presently aren't exposed to it

Would these environment variables control libcurl as well?

Goodness, we're just scrubbing misbehaviour like that in opam's libs, but I really hope not!

@rgrinberg
Copy link
Owner

@emillon I gave you merge access to proceed as you see fit.

@emillon
Copy link
Collaborator Author

emillon commented Mar 23, 2021

Thanks! I think that the most conservative option is to merge + release that PR does (this unlocks dune-release on windows for example) and keep the status quo for now.

Would these environment variables control libcurl as well?

Some of them do: https://curl.se/libcurl/c/libcurl-env.html

@jonahbeckford
Copy link
Collaborator

Can someone merge this PR? It works on Windows. Thx

@rgrinberg rgrinberg merged commit b46c5ff into rgrinberg:master Mar 7, 2022
emillon added a commit to emillon/opam-repository that referenced this pull request Jan 20, 2023
CHANGES:

* Passthrough case-insensitive PATH and SYSTEMROOT on Windows (@emillon,
  @jonahbeckford, rgrinberg/curly#6, rgrinberg/curly#8)
* Passthrough PATH to Unixes too. (@bikal, rgrinberg/curly#12)
* Add `?follow_redirects` argument to `run` and related functions
  (@rawleyfowler, rgrinberg/curly#5).
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants