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: improve version detection of installed modules #19933

Merged

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Nov 19, 2023

This PR adds a comparison of the head and the tag to improve detecting if an installed module was installed at a version. This helps to not interpret a module as "version-installed-module" whose latest commit-sha just temporarily matches a tag. But it can treat it as a module at its latest version.

Also fixes a typo made in the last PR.

🤖[deprecated] Generated by Copilot at d0b19a5

This pull request improves the vpm tool by fixing a typo in the issue template URL and using a more reliable method to detect the versions of installed modules.

🤖[deprecated] Generated by Copilot at d0b19a5

  • Fix typo in issue template URL for modules missing a manifest (link)
  • Improve module version detection by using git ls-remote --refs instead of git ls-remote --tags (link)

@@ -109,9 +109,23 @@ fn parse_query(query []string) ([]Module, []Module) {
}
}
mod.version = version
if v := os.execute_opt('git ls-remote --tags ${mod.install_path}') {
if refs := os.execute_opt('git ls-remote --refs ${mod.install_path}') {
Copy link
Member

Choose a reason for hiding this comment

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

The output of that command is a list of potentially many lines, each describing a different reference (i.e. potentially several tags, and several heads, if there are many branches).
The way it is processed below however, seems to me a bit brittle, since it expects a single of each (calling .all_after_last can return something containing many lines).

Copy link
Member

Choose a reason for hiding this comment

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

An example for ~/.vmodules/sdl:

#0 16:57:28 ᛋ master /v/vnew❱git ls-remote --refs ~/.vmodules/sdl/
73069e4c2b08af7712c28d58730de780f7d13f87        refs/heads/2.0.10
dde21987200582e837fe25281d918f2b959a695f        refs/heads/2.28.0
026c7041241096f02d2ad5d765db7fcce93ec01e        refs/heads/fix_compilation_with_gc_boehm_leak
20fde67f17b285b3a942731048b8ae6ab152d9d8        refs/heads/master
73069e4c2b08af7712c28d58730de780f7d13f87        refs/remotes/origin/2.0.10
a4fbceefb239155579a8f15feacd2d9dc54da621        refs/remotes/origin/2.0.12
8962fecffd1a6f93c2b05554fa0a0265d6d7bdff        refs/remotes/origin/2.0.14
98fd50ff11945766d98da4b7c47cff65c247112d        refs/remotes/origin/2.0.16
19078a02c83a1a192b126c2717a14ef0dbf9d2f8        refs/remotes/origin/2.0.18
f57e74a67ae06a2378b3c539d9c76a677e43d7a4        refs/remotes/origin/2.0.20
dbc80fd3c7603273c1c62c6108efeb7e3079efae        refs/remotes/origin/2.0.22
fddcc18043ce4a61b1b043336c098cf1f963e0c4        refs/remotes/origin/2.0.9
91be9010578212f197a996a1bef97ee77f87d8b0        refs/remotes/origin/2.24.0
2494be60dd7a6b1a2f85cf6e39dac3bd1b69f14e        refs/remotes/origin/2.26.0
dde21987200582e837fe25281d918f2b959a695f        refs/remotes/origin/2.28.0
20fde67f17b285b3a942731048b8ae6ab152d9d8        refs/remotes/origin/HEAD
026c7041241096f02d2ad5d765db7fcce93ec01e        refs/remotes/origin/fix_compilation_with_gc_boehm_leak
20fde67f17b285b3a942731048b8ae6ab152d9d8        refs/remotes/origin/master
1a02313b255414547830ad47b5c7fd951e000f83        refs/stash
#0 16:59:40 ᛋ master /v/vnew❱

Copy link
Member Author

@ttytm ttytm Nov 19, 2023

Choose a reason for hiding this comment

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

The output of that command is a list of potentially many lines, each describing a different reference (i.e. potentially several tags, and several heads, if there are many branches).
The way it is processed below however, seems to me a bit brittle, since it expects a single of each (calling .all_after_last can return something containing many lines).

You're right 👍 I should've handled the multiple lines here. Thanks @spytheman! I'm going to encapsulate just this installed-version detection. Then I can add a unit test for it, we have a mainly integration tests for vpm atm and wanted to add more unit tests anyway.

To add a special comment about this:

(i.e. potentially several tags, and several heads, if there are many branches).

An example for ~/.vmodules/sdl:

#0 16:57:28 ᛋ master /v/vnew❱git ls-remote --refs ~/.vmodules/sdl/
73069e4c2b08af7712c28d58730de780f7d13f87        refs/heads/2.0.10
dde21987200582e837fe25281d918f2b959a695f        refs/heads/2.28.0
026c7041241096f02d2ad5d765db7fcce93ec01e        refs/heads/fix_compilation_with_gc_boehm_leak
20fde67f17b285b3a942731048b8ae6ab152d9d8        refs/heads/master
73069e4c2b08af7712c28d58730de780f7d13f87        refs/remotes/origin/2.0.10
a4fbceefb239155579a8f15feacd2d9dc54da621        refs/remotes/origin/2.0.12
8962fecffd1a6f93c2b05554fa0a0265d6d7bdff        refs/remotes/origin/2.0.14
98fd50ff11945766d98da4b7c47cff65c247112d        refs/remotes/origin/2.0.16
19078a02c83a1a192b126c2717a14ef0dbf9d2f8        refs/remotes/origin/2.0.18
f57e74a67ae06a2378b3c539d9c76a677e43d7a4        refs/remotes/origin/2.0.20
dbc80fd3c7603273c1c62c6108efeb7e3079efae        refs/remotes/origin/2.0.22
fddcc18043ce4a61b1b043336c098cf1f963e0c4        refs/remotes/origin/2.0.9
91be9010578212f197a996a1bef97ee77f87d8b0        refs/remotes/origin/2.24.0
2494be60dd7a6b1a2f85cf6e39dac3bd1b69f14e        refs/remotes/origin/2.26.0
dde21987200582e837fe25281d918f2b959a695f        refs/remotes/origin/2.28.0
20fde67f17b285b3a942731048b8ae6ab152d9d8        refs/remotes/origin/HEAD
026c7041241096f02d2ad5d765db7fcce93ec01e        refs/remotes/origin/fix_compilation_with_gc_boehm_leak
20fde67f17b285b3a942731048b8ae6ab152d9d8        refs/remotes/origin/master
1a02313b255414547830ad47b5c7fd951e000f83        refs/stash
#0 16:59:40 ᛋ master /v/vnew❱

Please read:
This is a case where a module was changed manually in the .vmodules dir, it's not a shallow clone anymore that is only manged by vpm.

To recall the context of this PR: the change is related to calling the install command for an already installed module, and that it is correctly recognized when it is a "latest" version so that it can be recognized as updatable.

If there are miss-matches when re-installing a module, it would require --force or a confirmation, which I think is good.

Additionally, sdl is a special case which uses branches for version, but in that comment I won't carry out on this.

Copy link
Member

Choose a reason for hiding this comment

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

ok

@spytheman spytheman merged commit 8267874 into vlang:master Nov 19, 2023
42 checks passed
@ttytm ttytm deleted the tools/vpm/improve-installed-ver-detection 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.

2 participants