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

Always use httpclient in nimgrab #19767

Merged
merged 1 commit into from May 17, 2022
Merged

Always use httpclient in nimgrab #19767

merged 1 commit into from May 17, 2022

Conversation

ghost
Copy link

@ghost ghost commented May 5, 2022

Previously, nimgrab used the URLDownloadToFile function which seems to be quite old and, on another note, antiviruses seem to be really suspicious towards any applications using this API.

Also changed imports to use std and removed an unneeded asyncdispatch import as nimgrab always uses synchronous HttpClient.

@juancarlospaco
Copy link
Collaborator

What is actively using this?, is it the Windows installation process ?. 🤔

@ghost
Copy link
Author

ghost commented May 5, 2022

What is actively using this?, is it the Windows installation process ?. 🤔

Yes, it's used for downloading mingw. The discussion was in #internals on Discord

@Araq
Copy link
Member

Araq commented May 6, 2022

Who can test the resulting finish.exe on Windows?

@ringabout
Copy link
Member

ringabout commented May 16, 2022

Who can test the resulting finish.exe on Windows?

Tested on Win 11:

image

I changed the code to show the new nimgarb is actually used:

image

@ringabout ringabout requested a review from Araq May 16, 2022 11:47
@Araq Araq merged commit 06f02bb into nim-lang:devel May 17, 2022
@ringabout
Copy link
Member

ringabout commented Aug 8, 2022

@narimiran can this PR be backported to 1.6.x? This PR seems to be harmless and helps newcomers on windows.

1.6.6

image

devel

image

@narimiran
Copy link
Member

narimiran commented Jan 30, 2023

@narimiran can this PR be backported to 1.6.x?

Sorry, I have missed this.

Should I backport it now?

@ringabout
Copy link
Member

Yes, please, thanks!

narimiran pushed a commit that referenced this pull request Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants