geogebra: Update checkver script#17052
Conversation
WalkthroughReplaced HTML scraping in Changes
Sequence Diagram(s)sequenceDiagram
participant PS as PowerShell script
participant Http as HttpClient (System.Net.Http)
participant Remote as download.geogebra.org
PS->>Http: Ensure System.Net.Http loaded (PS version guard)
PS->>Http: Create HttpClient (AllowAutoRedirect = true)
PS->>Http: SEND HEAD https://download.geogebra.org/package/win-suite
Http->>Remote: HEAD request
Remote-->>Http: 3xx redirect(s) -> final URI (package filename)
Http-->>PS: Return final RequestUri
PS->>PS: Extract package name from Uri, parse version (regex)
PS-->>Console: Write-Output latest_version
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
All changes look good. Wait for review from human collaborators. geogebra
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@bucket/geogebra.json`:
- Around line 56-65: The script creates disposable objects ($http_handler,
$client, $request, $response) but never disposes them; update the checkver code
to ensure these IDisposable objects are disposed (e.g., wrap the call to
$client.SendAsync($request).Result in a try/finally or use PowerShell's using
statement) so that $response.Dispose(), $request.Dispose(), $client.Dispose(),
and $http_handler.Dispose() are always called even on error; reference the
symbols $http_handler, $client, $request, and $response when implementing the
disposal.
🧹 Nitpick comments (1)
bucket/geogebra.json (1)
63-63: Consider simplifying the regex for extracting the filename.The regex
.+(?<=/)([^/]+)$works but is unnecessarily complex. A simpler alternative would be more readable.♻️ Simpler alternative
- "$package_name = $response.RequestMessage.RequestUri.AbsoluteUri -replace '.+(?<=/)([^/]+)$', '$1'", + "$package_name = $response.RequestMessage.RequestUri.Segments[-1]",Or if you prefer to keep the regex approach:
- "$package_name = $response.RequestMessage.RequestUri.AbsoluteUri -replace '.+(?<=/)([^/]+)$', '$1'", + "$package_name = $response.RequestMessage.RequestUri.AbsoluteUri -replace '^.+/', ''",
|
/verify |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@bucket/geogebra.json`:
- Around line 62-65: The code currently disposes $response, $request, $client,
and $http_handler before reading $response.RequestMessage.RequestUri which can
cause ObjectDisposedException and also never disposes if SendAsync throws;
update the flow in the routine that calls $client.SendAsync($request).Result so
you first call SendAsync and extract $package_name/$latest_version from
$response.RequestMessage.RequestUri (using the existing regex logic), then move
the disposal of $response, $request, $client, $http_handler into a finally block
to ensure they are always disposed after extraction; ensure extraction happens
before disposal and that exceptions still trigger disposal in the finally.
|
All changes look good. Wait for review from human collaborators. geogebra
|
|
/verify |
|
All changes look good. Wait for review from human collaborators. geogebra
|
|
Maybe it would be simpler to use this instead? The result contains: $res.Headers.Location
/installers/6.0/suite/GeoGebraCalculator-Windows-Installer-6-0-913-0.exe |
In PowerShell 5.x, ┏[ ~]
└─> $response = Invoke-WebRequest -Uri 'https://download.geogebra.org/package/win-suite' -Method Head -MaximumRedirection 0
Security Warning: Script Execution Risk
Invoke-WebRequest parses the content of the web page. Script code in the web page might be run when the page is parsed.
RECOMMENDED ACTION:
Use the -UseBasicParsing switch to avoid script code execution.
Do you want to continue?
[Y] Yes [A] Yes to All [N] No [L] No to All [S] Suspend [?] Help (default is "N"):
┏[ ~]
└─> $response = Invoke-WebRequest -Uri 'https://download.geogebra.org/package/win-suite' -Method Head -MaximumRedirection 0 -UseBasicParsing
Invoke-WebRequest : The maximum redirection count has been exceeded. To increase the number of redirections allowed, supply a higher value to the
-MaximumRedirection parameter.
At line:1 char:13
+ $response = Invoke-WebRequest -Uri 'https://download.geogebra.org/pac ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : InvalidOperation: (System.Net.HttpWebRequest:HttpWebRequest) [Invoke-WebRequest], InvalidOperationException
+ FullyQualifiedErrorId : MaximumRedirectExceeded,Microsoft.PowerShell.Commands.InvokeWebRequestCommand
In PowerShell 7.x, ┏[ ~]
└─> $response = Invoke-WebRequest -Uri 'https://download.geogebra.org/package/win-suite' -Method Head -MaximumRedirection 0
Invoke-WebRequest: Response status code does not indicate success: 302 (Moved Temporarily).
┏[ ~]
└─> $response = Invoke-WebRequest -Uri 'https://download.geogebra.org/package/win-suite' -Method Head -MaximumRedirection 0 -SkipHttpErrorCheck
Invoke-WebRequest: The maximum redirection count has been exceeded. To increase the number of redirections allowed, supply a higher value to the -MaximumRedirection parameter.
Code simplicity alone should not be the sole objective. General applicability and stability must also be taken into consideration. |
|
o-l-a-v proposed a common solution, though I'm not sure if it works for GeoGebra. [System.Net.HttpWebRequest]::Create(
'<url>'
).GetResponse().ResponseUri.AbsoluteUrie.g., Lines 36 to 43 in 0934d97 I've been considering replacing the original approach for retrieving redirect links in Extras bucket. e.g., Lines 32 to 38 in 0934d97 |
|
By checking the PowerShell major version and conditionally executing Even without this logic, the manifest can still be successfully updated to the latest version in a PowerShell 7.x environment. Therefore, I believe that the more performant and modern |
|
/verify |
|
All changes look good. Wait for review from human collaborators. geogebra
|
Our workflow takes only 2-3 minutes to check versions for 2000+ packages, so I don't think we need to worry about the performance overhead of That's why I'm in favor of o-l-a-v's proposal to replace everything with a unified solution and finally move it into Scoop-Core. |
I also support integrating this functionality into Scoop Core. Whether While I personally prefer adopting more modern technologies, I recognize that not everyone shares this view, which is entirely reasonable. Please advise whether |
|
Thanks for understanding!
I'd vote for using |
|
/verify |
|
All changes look good. Wait for review from human collaborators. geogebra
|
Done. |
|
/verify |
|
All changes look good. Wait for review from human collaborators. geogebra
|
3 participants
Summary
Refactors the
checkverscript forgeogebrato improve maintainability and detection accuracy by tracking official download redirects.Related issues or pull requests
Changes
checkverscript:/6.0/directory) with a dynamicHEADrequest to the generic suite entry point (/package/win-suite).HttpClientwithAllowAutoRedirect = $trueto resolve the final download URL.AbsoluteUri.System.Net.Httpis loaded correctly.Notes
checkverwas prone to failure when the major/minor version changed (e.g., moving from 5.2 to 5.4). The new implementation is version-agnostic and relies on the official redirection service provided by GeoGebra.HEADrequest instead of a fullGETrequest (Invoke-WebRequest) is more efficient as it only retrieves metadata without downloading the entire page content.geogebra(v6) manifest with the updated logic applied togeogebra5.Testing
The test results are as follows:
<manifest-name[@version]|chore>: <general summary of the pull request>Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.