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

Outdated apps are not considered outdated by Scoop #4489

Closed
CrendKing opened this issue Oct 29, 2021 · 16 comments
Closed

Outdated apps are not considered outdated by Scoop #4489

CrendKing opened this issue Oct 29, 2021 · 16 comments

Comments

@CrendKing
Copy link
Contributor

CrendKing commented Oct 29, 2021

I have kicad-lite installed from the Extras bucket. Currently the latest version is 5.1.10_1, while my installed version is 5.1.9_1. scoop update doesn't update anything. Upon digging, I found that the compare_versions function is the culprit.

To reproduce, simply run compare_versions '5.1.10_1' '5.1.9_1' and you will get result -1, which means NOT outdated, which is wrong. The problem is basically '10_1' -gt '9_1' is false and '10_1 -lt '9_1' is true. Maybe we should pad "0"s in the front before comparing, such as

    for($i=0;$i -lt $ver_a.length;$i++) {
        if($i -gt $ver_b.length) { return 1; }

        $ver_a_str = [string] $ver_a[$i]
        $ver_b_str = [string] $ver_b[$i]

        $padded_length = ($ver_a_str.Length, $ver_b_str.Length | Measure-Object -Maximum).Maximum
        $padded_a = $ver_a_str.PadLeft($padded_length, '0')
        $padded_b = $ver_b_str.PadLeft($padded_length, '0')

        if($padded_a -gt $padded_b) { return 1; }
        if($padded_a -lt $padded_b) { return -1; }
    }

I can create a pull request if this is OK.

@CrendKing CrendKing changed the title compare_versions is returning wrong result Outdated apps are not considered outdated by Scoop Oct 29, 2021
@rashil2000
Copy link
Member

@niheaven could you take a look at this?

@batkiz
Copy link

batkiz commented Nov 6, 2021

ffmpeg got a similar issue.

The lastest version of ffmpeg is 4.4-190, I got version 4.4.1-2 installed on my PC.

While checking outdated apps, ffmpeg is NOT marked as outdated, the problem is before compare_versions, the version number would be passed to func version and got splited by . or -, but the code treats . and - all the same, so 4.4-190 will be 4 4 190 and 4.4.1-2 will be 4 4 1 2. That's why compare_versions 4.4.1-2 4.4-190 got -1.

Maybe we should optimize the version func to get a better version number split experience.

@batkiz
Copy link

batkiz commented Nov 6, 2021

not familiar with pwsh, but try to make a quick & dirty fix

class VersionPoc {
    [string] $Main
    [string] $Sub
}

function GetVersion( $ver ) {
    $arr = $ver -split '[-_]'
    $v = New-Object VersionPoc
    $v.Main = $arr[0]
    if ($arr.count -gt 1) {
        $v.Sub = $arr[1]
    }
    return $v
}


function version($ver) {
    $ver -split '[\.]' | ForEach-Object {
        $num = $_ -as [int]
        if($num) { $num } else { $_ }
    }
}
function compare_versions($a, $b) {
    $ver_a = @(version $a)
    $ver_b = @(version $b)


    for($i=0;$i -lt $ver_a.length;$i++) {
        if($i -gt $ver_b.length) { return 1; }

        # don't try to compare int to string
        if($ver_b[$i] -is [string] -and $ver_a[$i] -isnot [string]) {
            $ver_a[$i] = "$($ver_a[$i])"
        }

        if($ver_a[$i] -gt $ver_b[$i]) { return 1; }
        if($ver_a[$i] -lt $ver_b[$i]) { return -1; }
    }
    if($ver_b.length -gt $ver_a.length) { return -1 }
    return 0
}


function cmpver($a, $b) {
    $res = compare_versions GetVersion($a).Main, GetVersion($b).Main
    if ($res -ne 0) { return $res }

    return compare_versions GetVersion($a).Sub, GetVersion($b).Sub
}

cmpver  4.4.1-2 4.4-190
cmpver '5.1.10_1' '5.1.9_1'

execution result:

1
1

However, the version number is very variable, sometimes even git commit sha would appears. I think we should do a well-rounded review on version numbers in scoop manifests, then we will know how to cover more cases.

@batkiz
Copy link

batkiz commented Nov 6, 2021

data from main, extra and dorado bucket. do not include versions full of digit, . , - or _

app version
aapt2 7.2.0-alpha03-7832930
firefox-beta-zh-cn 94.0b4
firefox-developer-zh-cn 72.0b11
firefox-nightly-zh-cn 96.0a1.20211105214712
hfs 2.3m
mcedit 2.0.0-beta14
obs-classic 0.659b
openethereum 3.3.0-rc.8
powershell-preview 7.2.0-rc.1
s2cb 1.13c
snipaste-beta 2.6.6-Beta
sunvox 1.9.6c
swift-snapshot snapshot-2021-10-27-a
winlibs-mingw-snapshot 11.2.1-9.0.0-snapshot20210814-r1
winlibs-mingw 11.2.0-12.0.1-9.0.0-r1
apache-directory-studio 2.0.0.v20210717-M17
axiom 1.9.6.1-alpha
banana-cake-pop 1.0.0-preview.3
baretail 3.50a
better-joy 6.4b
chromium-dev-nosync 86.0.4185.0-r769185
chromium-dev 91.0.4467.0-r869045
chromium-nosync 67.0.3396.99-r550428
cmake-rc 3.22.0-rc1
conemu-color-themes .june.2020.2
dart-dev 2.15.0-268.0.dev
dartium-content-shell-dev 1.25.0-dev.6.0
dartium-dev 1.25.0-dev.6.0
emule-community 0.60c
emule 0.50a
fbide 0.4.6r4
firefox-beta 94.0b4
firefox-developer 95.0b3
firefox-nightly 96.0a1.20211105214712
flashboot 3.3j
freetube 0.15.0-beta
gatling 3.7.0-M4
gcc-arm-none-eabi 10-2020-q4-major
goldendict 1.5.0-RC2-372-gc3ff15f
gretl 2021d
hain 0.7.0-beta.1
infoqube 0.9.116m
ipfilter-updater 3.0.2.9-beta
lens 5.2.6-latest.20211104.1
m4a-to-mp3-converter X.89
outlook-google-calendar-sync 2.9.0-beta
phraseexpress 15.0.86d
pietty 0.4.00b14
plex-desktop 1.35.1.2632-c6783c78
plex-player 2.58.0.1076-38e019da
pocketsphinx 5prealpha
prime95 30.3b6
sphinxbase 5prealpha
sphinxtrain 5prealpha
status 190516-062034-d94424
stoplight-studio 2.3.0-stable.6547.git-2cf5b62
sysinternals 2021.October.26
task-coach 1.4.6_rev1
testmace 0.0.1-beta.19
tremulous 1.3.0-alpha.0.14
uniextract2 2.0.0-rc.3
vboxvmservice 6.1-Kiwi
wezterm 20210814-124438-54e29167
write 1.0b8.4
wumgr 1.1b
yatqa 3.9.8.3f
zeronet 670a9cf2e737f6b11187129515b2d4b73c78c962

@CrendKing
Copy link
Contributor Author

CrendKing commented Nov 6, 2021

@batkiz Nice try, but it still only handles - and _. What if some developer decides to use another weird character?

Scoop is a system that tries to "receive" all software out there. It is impossible to create ONE algorithm that describes version comparison for every single app. What I'm thinking of is that, Scoop should decide to support one versioning scheme (e.g. semver), and allow bucket maintainer to convert any non-standard version to this scheme. The conversion could be arbitrary, as long as it's ordered.

For example, suppose we have wezterm's two versions: 20210814-124438-54e29167 and 20211020-3854720-48a48e0. The maintainer could convert each to something like "2021.8.14-1628981256" and "2021.10.20-1634770056". The last segment can be the epoch timestamp of when the manifest gets updated for ordering. Then it's easy to compare these two.

If it's so weird, like zeronet's 670a9cf2e737f6b11187129515b2d4b73c78c962, probably just do 0.0.0-<timestamp_of_update>.

If the version is already standard, obviously no conversion is needed.

@batkiz
Copy link

batkiz commented Nov 7, 2021

@CrendKing

What I'm thinking of is that, Scoop should decide to support one versioning scheme (e.g. semver), and allow bucket maintainer to convert any non-standard version to this scheme.

It's confusing if the scoop version is different with the official version for users, if in this way, maybe an internal version field should be introduced to scoop.

I'm thinking maybe we could add version compare field to manifest, like pre_install, it's powershell script but should follow some rules, just like the compare_versions func do. In this way, scoop just need to handle some of the most widely-used version schemes(e.g. semver), and other wired version schemes could be handled case by case.

@CrendKing
Copy link
Contributor Author

CrendKing commented Nov 7, 2021

The version I mentioned is internal. I was thinking of a new "version_conversion" field in the manifest that defines a function, where Scoop would call this function with the $version (which is public to user) as input and use the output for comparison (which could never be shown to user). The compare_version only handles standard semver.

Later, Scoop could even pre-define some common conversion function, like "convert arbitrary string-like version to 0.0.0-<timestamp>", give these functions string names, then maintainer can simply put "version_conversion": "timestamp".

Basically what you said, but instead of pre_install, I thought it to be an official, stand-alone new field.

@batkiz
Copy link

batkiz commented Nov 7, 2021

but instead of pre_install, I thought it to be an official, stand-alone new field.

that's what I thought, I meant it's value would be powershell script like pre_install.

@niheaven
Copy link
Member

niheaven commented Nov 8, 2021

Ref: #3721

Please wait a moment for this PR being merged 😢

@CrendKing
Copy link
Contributor Author

CrendKing commented Nov 15, 2021

@niheaven I see the PR is merged. However, if I test Compare-Version '5.1.9_1' '5.1.10_1' it's still returning -1, and the only test containing _ has it only on one side. Can you update the test cases to have '5.1.9_1' '5.1.10_1' and make it work?

@rashil2000
Copy link
Member

The PR was merged into develop branch, which is for testing. What is the output of scoop config SCOOP_BRANCH?

@CrendKing
Copy link
Contributor Author

No worry. I'm using main branch. I just want to chime in before it goes main. I see there are many test cases, but it bounds to miss some, like 5.1.9_1. I wonder what's the plan to come up with a exhaustive solution?

@niheaven
Copy link
Member

@CrendKing Fixed in ae89213

@CrendKing
Copy link
Contributor Author

Thank you for the quick fix. However, may I suggest that instead of comparing "_8" against "_9", compare "_10" against "_9", because the latter would fail if the function is a ASCII comparison (like before)? It would catch more potential bugs and regressions.

@niheaven
Copy link
Member

Added it, thanks. Any other problem or this should be closed 😄

@CrendKing
Copy link
Contributor Author

I believe you. Thanks!

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

No branches or pull requests

4 participants