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

tools.vpm: support again http installs when installing from an url (workaround) #19914

Merged
merged 3 commits into from
Nov 18, 2023

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Nov 17, 2023

Until we make a decision on eventual changes of the default install location of url installs this enables http installs to work like https installs.

🤖[deprecated] Generated by Copilot at 436b7c4

Improve vpm tool by refactoring tests, adding a new test case, and supporting both https and http URLs for module installation.

🤖[deprecated] Generated by Copilot at 436b7c4

  • Enable vpm to install modules from http URLs (link)
  • Move test_install_from_git_url to common.v to avoid duplication (link)
  • Add test case for installing vsl from https URL (link)

@spytheman
Copy link
Member

Please also add a test for v install http://github.com/Wertzui123/HashMap. The current changes to the test, do not reflect the change in the source code of VPM.

@JalonSolov
Copy link
Contributor

One could argue that we shouldn't be supporting http urls at all, any longer... but as long as we do, they should actually work.

@ttytm
Copy link
Member Author

ttytm commented Nov 17, 2023

One could argue that we shouldn't be supporting http urls at all, any longer... but as long as we do, they should actually work.

Yep, http://github.com/Wertzui123/HashMap isn't a valid github url. If you try to do a regular clone it throws a warning and redirects to https.

I'm fine with not having it merged but when we say http support has ended instead.

@spytheman
Copy link
Member

One could argue that we shouldn't be supporting http urls at all, any longer... but as long as we do, they should actually work.

Yes, but we are using it here as a workaround, for a more fundamental problem, that will need lots of discussions and decisions, while @Wertzui123 and lots of others that depend on that quirk will be waiting/blocked otherwise :-| .

Github redirects to https anyways:

#0 17:31:05 ᛋ master /v/vnew❱curl -vvv http://github.com/Wertzui123/HashMap
*   Trying 140.82.121.3:80...
* TCP_NODELAY set
* Connected to github.com (140.82.121.3) port 80 (#0)
> GET /Wertzui123/HashMap HTTP/1.1
> Host: github.com
> User-Agent: curl/7.68.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 301 Moved Permanently
< Content-Length: 0
< Location: https://github.com/Wertzui123/HashMap
< 
* Connection #0 to host github.com left intact
#0 17:31:15 ᛋ master /v/vnew❱

@spytheman spytheman changed the title tools: support http installs when installing from url tools: support again http installs when installing from an url (workaround) Nov 17, 2023
@spytheman spytheman changed the title tools: support again http installs when installing from an url (workaround) tools.vpm: support again http installs when installing from an url (workaround) Nov 17, 2023
@ttytm
Copy link
Member Author

ttytm commented Nov 17, 2023

Updated as discussed. Though with the deprecation warning of http while https behaving differently it's suboptimal user experience, we should align things asap 👍

@ttytm
Copy link
Member Author

ttytm commented Nov 18, 2023

@Wertzui123 Regarding the deprecation of http: to retain the behavior of the install location and resolve the warning, you can already register the module as vpm module and install it directly via the identifier v install wertzui123.hashmap.

Restoring the old behavior imho remains restoring an exploit. URL installs were programmed with the intention to be installed into the global namespace. That's how URL installs were understood and used by common modules like v-analzyer. It was a bug that paths of http installs weren't resolved correctly.

name := get_name_from_url(ident) or {
is_http := if ident.starts_with('http://') {
vpm_warn('installing `${ident}` via http.
${' '.repeat('warning: '.len)}`http` support is deprecated, switch to `https` to ensure future compatibility.')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just use vpm_warn 2 times?

@spytheman spytheman merged commit fb3d0cd into vlang:master Nov 18, 2023
42 checks passed
@Wertzui123
Copy link
Contributor

@Wertzui123 Regarding the deprecation of http: to retain the behavior of the install location and resolve the warning, you can already register the module as vpm module and install it directly via the identifier v install wertzui123.hashmap.

Restoring the old behavior imho remains restoring an exploit. URL installs were programmed with the intention to be installed into the global namespace. That's how URL installs were understood and used by common modules like v-analzyer. It was a bug that paths of http installs weren't resolved correctly.

Ok thank you. I published the library back when VPM was broken for a while so I never registered it there.

Yet I still think URL installs should not install the module to the global namespace and instead they should contain the authors name.

@ttytm
Copy link
Member Author

ttytm commented Nov 19, 2023

Yet I still think URL installs should not install the module to the global namespace and instead they should contain the authors name.

Yes, it likely will go in that direction. For now - with the recently added version install feature - the tiny advantage of two different install locations is that you can install different versions already, one via url one via the vpm registry. Handling of multiple installed versions under a central install location will eventually be implemented, it's not a prio atm.

@ttytm ttytm deleted the tools/vpm/http-url-installs branch December 15, 2023 22:08
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