Skip to content

Properly sort versions in "emsdk list"#508

Merged
sbc100 merged 1 commit into
emscripten-core:masterfrom
orestisfl:emsdk-list
Jun 1, 2020
Merged

Properly sort versions in "emsdk list"#508
sbc100 merged 1 commit into
emscripten-core:masterfrom
orestisfl:emsdk-list

Conversation

@orestisfl
Copy link
Copy Markdown
Contributor

This uses LooseVersion from distutils, here is the doc:

Version numbering for anarchists and software realists. Implements the
standard interface for version number classes as described above. A
version number consists of a series of numbers, separated by either
periods or strings of letters. When comparing version numbers, the
numeric components will be compared numerically, and the alphabetic
components lexically.
...
In fact, there is no such thing as an invalid version number under
this scheme; the rules for comparison are simple and predictable, but
may not always give the results you want (for some definition of
"want").

I believe that this should be available in all possible configurations of python, I checked with python2 & 3 on linux. If not, an easy workaround is to catch ImportError and set LooseVersion=None.

Comparison:
    Before   |   After
    1.39.9       1.39.16
    1.39.8       1.39.15
    1.39.7       1.39.14
    1.39.6       1.39.13
    1.39.5       1.39.12
    1.39.4       1.39.11
    1.39.3       1.39.10
    1.39.2       1.39.9
    1.39.16      1.39.8
    1.39.15      1.39.7
    1.39.14      1.39.6
    1.39.13      1.39.5
    1.39.12      1.39.4
    1.39.11      1.39.3
    1.39.10      1.39.2
    1.39.1       1.39.1
    1.39.0       1.39.0
    1.38.48      1.38.48
    1.38.47      1.38.47
    1.38.46      1.38.46
    1.38.45      1.38.45
    1.38.44      1.38.44
    1.38.43      1.38.43
    1.38.42      1.38.42
    1.38.41      1.38.41
    1.38.40      1.38.40
    1.38.39      1.38.39
    1.38.38      1.38.38
    1.38.37      1.38.37
    1.38.36      1.38.36
    1.38.35      1.38.35
    1.38.34      1.38.34
    1.38.33      1.38.33

@sbc100
Copy link
Copy Markdown
Collaborator

sbc100 commented May 28, 2020

Great!

Sadly I believe some distros such as Ubuntu stopped shipping distutils. We recently had to remove a use of distutils from emscripten itself for this reason: emscripten-core/emscripten#10943.

Perhaps a basic re-implementation would be better here?

@orestisfl
Copy link
Copy Markdown
Contributor Author

orestisfl commented May 28, 2020

Wow that's absurd.

I see the possible alternatives, please select one:

  • Try-catch ImportError, set LooseVersion=None (or use a variable to store the sorting key) will fall-back to default sorting
    • Easiest but fall-back is suboptimal
  • Try-catch ImportError, fall-back to custom implementation
    • Essentially split at dots, convert each element to int, do not panic at non-int strings
  • Custom implementation without even trying to import LooseVersion

@sbc100
Copy link
Copy Markdown
Collaborator

sbc100 commented May 28, 2020

I bet we can write a good enough version of this in 3 or 4 lines of python, no?

It doesn't need to handle all the possible cases... we could probably start with something like: lst.sort(key=lambda s: [int(x) for x in s.split('.')]).. maybe not quite that simple but its starting point.

@sbc100 sbc100 closed this May 28, 2020
@sbc100 sbc100 reopened this May 28, 2020
Split version to integers but avoid exceptions in case there is a
non-numerical character in some version.

Comparison:
    Before   |   After
    1.39.9       1.39.16
    1.39.8       1.39.15
    1.39.7       1.39.14
    1.39.6       1.39.13
    1.39.5       1.39.12
    1.39.4       1.39.11
    1.39.3       1.39.10
    1.39.2       1.39.9
    1.39.16      1.39.8
    1.39.15      1.39.7
    1.39.14      1.39.6
    1.39.13      1.39.5
    1.39.12      1.39.4
    1.39.11      1.39.3
    1.39.10      1.39.2
    1.39.1       1.39.1
    1.39.0       1.39.0
    1.38.48      1.38.48
    1.38.47      1.38.47
    1.38.46      1.38.46
    1.38.45      1.38.45
    1.38.44      1.38.44
    1.38.43      1.38.43
    1.38.42      1.38.42
    1.38.41      1.38.41
    1.38.40      1.38.40
    1.38.39      1.38.39
    1.38.38      1.38.38
    1.38.37      1.38.37
    1.38.36      1.38.36
    1.38.35      1.38.35
    1.38.34      1.38.34
    1.38.33      1.38.33
@orestisfl
Copy link
Copy Markdown
Contributor Author

Up, WDYT?

@orestisfl
Copy link
Copy Markdown
Contributor Author

This doesn't handle all possible cases (but at least it doesn't crash) and it should be enough for the versioning used in this project

@kripken
Copy link
Copy Markdown
Member

kripken commented Jun 1, 2020

This looks good to me, wdyt @sbc100 ?

@sbc100 sbc100 merged commit 6b0d151 into emscripten-core:master Jun 1, 2020
@orestisfl orestisfl deleted the emsdk-list branch June 2, 2020 08:46
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.

3 participants