Closed ipcjs closed 6 months ago
Hi @HUMORCE, could you please help me review the PR?
The PR handler of https://github.com/ScoopInstaller/Extras/pull/12216 does not even execute to Test-PRFile
. For compatibility, then the action should be executed with windows powershell directly.
emm, I don't know much about this repo.
this is veify result: https://github.com/ScoopInstaller/Extras/pull/12216#issuecomment-1825990739
Test-PRFile
should have been executed.🤔️
The log output shows that it returned at L308. Try create a test repo for this.
This improvement should be implemented in Scoop Core / CI.
It already contains most of the relevant checks, but seems the JSON syntax check is not included.
Running tests from 'D:\a\Main\Main\my_bucket\Scoop-Bucket.Tests.ps1'
Describing Code Syntax
[+] PowerShell code files do not contain syntax errors 62ms (39ms|23ms)
Describing Style constraints for non-binary project files
[+] files do not contain leading UTF-8 BOM 474ms (471ms|3ms)
[+] files end with a newline 38ms (37ms|1ms)
[+] file newlines are CRLF 100ms (99ms|1ms)
[+] files have no lines containing trailing whitespace 121ms (120ms|1ms)
[+] any leading whitespace consists only of spaces (excepting makefiles) 126ms (125ms|1ms)
Describing Manifest validates against the schema
[+] D:\a\Main\Main\my_bucket\bucket\ptags.json 143ms (139ms|4ms)
Tests completed in 1.83s
Tests Passed: 7, Failed: 0, Skipped: 0 NotRun: 0
https://github.com/ScoopInstaller/Main/actions/runs/7063544527/job/19229759812#step:4:20
FYI I'm disabling scoop in Repology until proper validation is implemented, as less than a week from the previous fix, it's broken again.
bucket/avspmod.json: ERROR: parsing failed (fatal): JSONDecodeError: Expecting property name enclosed in double quotes: line 11 column 9 (char 503)
Shelling out to another process does not seem like the most efficient way to do this - is it possible to use a .NET API to do this directly (and uniformly)?
@rashil2000 I had a go at finding a .NET API to validate the JSON. However, I couldn't find a uniform way of doing it in both Powershell 5 and 7. This is because ConvertFrom-Json
uses System.Web.Script.Serialization.JavaScriptSerializer
as the underlying API in Powershell 5, but it uses System.Text.Json.JsonDocument
in Powershell 7. I couldn't load System.Text.Json.JsonDocument
in Powershell 5.
The following code checks the Powershell version and uses the right API. However, Microsoft's docs suggest that you should use System.Text.Json.JsonDocument
to parse JSON and Newtonsoft.Json
for versions that can't - so I'm not sure if we should use this:
$file = Get-Content $manifest -Raw
if ($PSVersionTable.PSVersion -lt "6.0") {
Add-Type -AssemblyName System.Web.Extensions
$jsonSerializer = New-Object System.Web.Script.Serialization.JavaScriptSerializer
try {
$parsed = $jsonSerializer.DeserializeObject($file)
$object = $file | ConvertFrom-Json
# Valid JSON
}
catch [System.ArgumentException] {
$object = $null
# Bad JSON
}
}
else {
try {
$parsed = [System.Text.Json.JsonDocument]::Parse($file)
$object = $file | ConvertFrom-Json
# Valid JSON
}
catch {
$object = $null
# Bad JSON
}
}
This code returns the parsed $object
if the manifest is valid JSON, and $object
is $null
otherwise. I tested this with both the mcomix
manifest that has trailing commas and without.
@rasa @rashil2000 @HUMORCE
I'm waiting for Scoop being brought back to the Repology list, and this PR is the primary blocker from what I can see. I believe there is one easier way to achieve this by just setting up the GitHub CI to use powershell
as the execution shell of the CI jobs, pwsh
is used by default according to GitHub Actions doc^1. EDIT: With this, no code in this repo needs to be changed. there is a mistake, I believe only here needs to be changed.
@chawyehsu Unfortunately, the CI jobs is not compatible with powershell.exe
.🫠
I haven't tested this repo on powershell
, I'll review the codebase to see what's incompatible and I think it would be more acceptable to me fixing compatiblity issues compared to shelling out to another process in this case.
Not much work needs to be done to switch to the powershell
shell for CI jobs to work.
trigger: https://github.com/chawyehsu/ScoopTestBucket/pull/2#issuecomment-1951315119
codebase diff: https://github.com/ScoopInstaller/GithubActions/compare/main...chawyehsu:GithubActions:powershell
The
ConvertFrom-Json
method in PowerShell 7 supports JSON with comments. TheConvertFrom-Json
method in PowerShell 5 supports only standard JSON. See the documentation for details: ConvertFrom-JsonFormatting the manifest file again using the
ConvertFrom-Json
method in PowerShell 5 ensures that the manifest file is in standard JSON format (without extra commas, comments, etc.).Relates to https://github.com/ScoopInstaller/Extras/pull/12216