Skip to content
This repository has been archived by the owner on Jan 27, 2020. It is now read-only.

Update views.py #4

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

valanc
Copy link

@valanc valanc commented Apr 25, 2016

Modify condition checking for lower and higher condition types:

  • enable version comparison for non numerical versions
  • use OR logical operator if multiple software is matching condition

@dam09fr
Copy link

dam09fr commented May 8, 2016

Hello, thank you to suggest improvements.
I quite agree with the proposed change to manage non numerical versions.
For now I've not tested the logical operator to OR if multiple software is matching condition.

I suggest you to make two different pull request, so any of the change can be analyzed independently.

@dam09fr
Copy link

dam09fr commented May 9, 2016

In addition to my previous comment, I would add that there are more "native methods" in Python to compare software versions. I think we should turn the implementation in this direction because for now the version comparison between a 1.0 and 1.0.1 does not work (it stops at the second number from my reading of the code).

Here is a simple code example:

from distutils.version import LooseVersion as ComparableVersion

installedversion = "1.0"
currentversion = "1.0.1"

if ComparableVersion(currentversion) > ComparableVersion(installedversion):
    print "Update available"
else:
    print "Application is up to date"

@valanc
Copy link
Author

valanc commented May 9, 2016

Bonjour Damien,

Je préfère répondre en français... Désolé s'il y a des anglophones intéressés !

Je ne suis pas expert en Python et j'ai tenu à conserver l'esprit du code original.
Concernant la comparaison de versions, je suis parti du constat que certaines portions de codes étaient incohérentes :

  • vclean = re.sub('[^.0-9]','',s.version) supprime les caractères autres que chiffres ou points du numéro de version installée
  • pourtant rien n'empêche d'avoir d'autres caractères dans les numéros de version des conditions d'installation
  • de plus, le code après except ValueError: n'avait alors aucun intérêt

Mais la suggestion d'utiliser la classe ComparableVersion me semble intéressante.
Comment ça se comporte avec des numéros de versions un peu moins standard ?
Par exemple : 4.0.5.0-2015.07.31 (X2Go)

Alain

@valanc
Copy link
Author

valanc commented May 9, 2016

Je poursuis avec le traitement des conditions de type lower (Logiciel non installé ou version inférieure à :) ou higher (Logiciel installé et version supérieure à :).

Prenons un cas concret avec une machine sur laquelle sont installés les logiciels suivants :

  • Java 8 Update 71
  • Java 8 Update 91
    La condition d'installation "Java 8 Update %" non installé ou version inférieure à 8.0.910 ne devrait-elle pas retourner False ?

Ces 2 types de conditions ont un fonctionnement qui me semble incohérent avec les autres !
Pour rester cohérent, il ne faut pas basculer la variable à False au début du test, mais uniquement si une version supérieure ou égale est déjà installé.

J'y ai ajouté quelques améliorations :

  • avant de sortir de la boucle, après la dernière comparaison (i == looplimit - 1), si len(vtab) >= len(condversiontab), on renvoie False. Ce qui permet de gérer à la fois l'égalité et un numéro de version installée plus long que celui de la condition (par exemple 1.0.1 installé pour une condition 1.0)
  • j'ai factorisé les messages d'erreur et ajouté l'instruction exit pour sortir de la boucle si Install = False

Je suppose que le code pourrait encore être optimisé. Par exemple en remplaçant les lignes suivantes :

if len(condversiontab) > len(vtab):
    looplimit = len(vtab)
else:
    looplimit = len(condversiontab)

par une seule instruction du genre :
looplimit = min(len(vtab), len(condversiontab))

Cette correction évite bien des cas de bouclage !
Par exemple si une ancienne version d'un logiciel est installée en 32-bits, la nouvelle version 64-bits s'installe souvent en parallèle. Si on l'applique à un profil, la version antérieure étant trouvée UpdatEngine réinstalle indéfiniment le paquet...

Alain

@dam09fr
Copy link

dam09fr commented May 9, 2016

On peut continuer en français, ça ne me pose pas de problème !
Le problème avec la comparaison des numéros de version est l'absence d'une normalisation globale suivie par les éditeurs / projets !

Attention, la classe ComparableVersion dans le code proposé est un alias pour la classe LooseVersion (j'ai ajouté l'abstraction pour pouvoir changer de classe sans modifier le code qui suit).

La dernière implémentation Python en date concernant les numéros de version est la suivante : NormalizedVersion. LooseVersion semble être la version la plus permissive et applicable en l'état, il existe également l'implémentation StrictVersion qui semble totalement inapplicable dans le cas de ce logiciel.
L'ensemble du fonctionnement de ces implémentations est documenté ici : https://www.python.org/dev/peps/pep-0386/

Le comportement de LooseVersion avec X2go ne me semble pas poser de problèmes particuliers d'après ce que je peux en voir. Il faudrait que tu fasse des tests à l'aide du script que j'ai fourni en utilisant des numéros de versions de l'application pour confirmer ça.

La limite de LooseVersion c'est la non prise en charge des versions "beta", "dev" ou autres qui seront certainement priorisés aux versions de release (sans notation spécifique). Hormis cela, je pense que ça couvre la majorité des cas.

Concernant ton second message ...

Ton analyse avec Java (2 versions installées) est correcte. Le fait que la version supérieure ou égale soit installée devrait empêcher le déclenchement de l'installation. Cependant la plupart de utilisateurs qui (comme moi) n'utilisent qu'une seule version ne rencontrent pas ce problème ...
Me concernant j'exploite le test sur le libellé de l'application : la valeur du champ est fixée à "Java 8 Update 92" et je laisse le champ version vide (et non pas undefined qui semble poser quelques problèmes). Ça doit répondre à ta problématique si je ne me trompes pas.

Pour la gestion des applications en 32 / 64 bits, je gère ça de plusieurs façons en fonction de l'application :

  • il y a marqué "64 bits" ou équivalent dans le libellé de l'application et je me base là-dessus pour filtrer dans les conditions d'installation (ex : Java, 7-zip)
  • j'utilise dans les conditions d'installation + un filtre Windows 64 bits ou Windows 32 bits pour affecter correctement les paquets en fonction de l'environnement de destination
  • j'insère un script qui désinstalle toute version du logiciel présente sur la machine (script powershell avec test par regex) avant de lancer l'installation de l'application souhaitée ce qui évite les doublons

J'ai besoin de monter un environnement de développement côté serveur pour aller plus loin dans la réflexion et les tests (ça commence à me filer mal à la tête) je reviens vers toi prochainement.

@valanc
Copy link
Author

valanc commented May 10, 2016

Bonjour,

Merci, c'est très intéressant. J'apprends plein de chose sur Python !

Il y aurait effectivement une simplification non négligeable du code en utilisant cette comparaison de version. J'ai trouvé sur la page http://www.programcreek.com/python/example/6084/distutils.version.LooseVersion des exemples de comparaison de versions dont :

def compare_versions(version1, version2):
    try:
        return cmp(StrictVersion(version1), StrictVersion(version2))
    # in case of abnormal version number, fall back to LooseVersion
    except ValueError:
        pass
    try:
        return cmp(LooseVersion(version1), LooseVersion(version2))
    except TypeError:
    # certain LooseVersion comparions raise due to unorderable types,
    # fallback to string comparison
        return cmp([str(v) for v in LooseVersion(version1).version],
                   [str(v) for v in LooseVersion(version2).version])

Le code pour views.py pourrait alors être :

    # Software not installed or version lower than (one wildcard can be used for condition name)
    if install == True:
        for condition in pack.conditions.filter(depends='lower'):
            if '*' in condition.softwarename:
                nametab = condition.softwarename.split('*')
                if software.objects.filter(host_id=m.id, name__startswith=nametab[0],name__endswith=nametab[1]).exists():
                    softtab = software.objects.filter(host_id=m.id, name__startswith=nametab[0],name__endswith=nametab[1])
                    if compare_versions(softtab, condition.softwareversion) < 0:
                        install = False
            else:
                if software.objects.filter(host_id=m.id, name=condition.softwarename).exists():
                    softtab = software.objects.filter(host_id=m.id, name=condition.softwarename)
                    if compare_versions(softtab, condition.softwareversion) < 0:
                        install = False
            if install == False:
                status(''+str(m.id)+''+str(pack.id)+'Warning condition: '+escape(condition.name)+'')
    # Software installed and version higher than
    if install == True:
        for condition in pack.conditions.filter(depends='higher'):
            if '*' in condition.softwarename:
                nametab = condition.softwarename.split('*')
                if software.objects.filter(host_id=m.id, name__startswith=nametab[0],name__endswith=nametab[1]).exists():
                    softtab = software.objects.filter(host_id=m.id, name__startswith=nametab[0],name__endswith=nametab[1])
                    if compare_versions(softtab, condition.softwareversion) > 0:
                        install = False
                else:
                    install = False
            else:
                if software.objects.filter(host_id=m.id, name=condition.softwarename).exists():
                    softtab = software.objects.filter(host_id=m.id, name=condition.softwarename)
                    if compare_versions(softtab, condition.softwareversion) > 0:
                        install = False
                else:
                    install = False
            if install == False:
                status(''+str(m.id)+''+str(pack.id)+'Warning condition: '+escape(condition.name)+'')

Je testes ça dès que possible !

@valanc
Copy link
Author

valanc commented May 10, 2016

Une condition avec version undefined signifie bien que la version installée n'est pas définie (champ vide). En revanche, je ne savais pas ou j'avais oublié qu'il était possible d'avoir une condition avec champ vide pour autoriser n'importe quel numéro de version (ça doit être géré au niveau des filtres).

Outre Java (désormais plus problématique car j'ai un script pour désinstaller toute version de Java 8 avant l'installation d'une nouvelle version), j'ai rencontré le problème d'installations multiples avec "7-Zip *" et "VLC media player". Soit à cause de l'installation de versions 32-bits et 64-bits (nous partons d'un parc très hétérogène), soit parce que l'installation de la nouvelle version n'a pas supprimé ou remplacé l'ancienne (si je me souviens bien, c'est le cas de 7-Zip 15.x qui laisse la version 9.20).

Toujours est-il qu'avec ce problème de test, un accident est vite arrivé. Si plusieurs centaines de machines installent un paquet en boucle, c'est pas terrible pour le serveur et le réseau ! De plus, le code existant n'était pas optimum : si dans un cas bien particulier vous aviez plusieurs conditions de type higher ou lower, il suffisait qu'une seule soit vrai pour que l'installation se fasse (première boucle for condition...) !

@dam09fr
Copy link

dam09fr commented May 10, 2016

La fonction "compare_versions" que tu as postée, me semble être une implémentation très intéressante, je vais également faire quelques tests (en particulier sur les types d'erreurs gérées dans les except) !

Ok avec ton 2nd message, j'ai fais les mêmes constatations de mon côté.

dam09fr added a commit to dam09fr/updatengine-server that referenced this pull request May 10, 2016
See : yvguim#4
- change version compare implementation by using standard Python class (StrictVersion + LooseVersion + String compare)
- use OR logical operator if multiple software is matching condition
@dam09fr
Copy link

dam09fr commented May 10, 2016

Je suis parti de ton travail, voici le résultat : https://github.com/dam09fr/updatengine-server/tree/dam09fr-patch-versioncompare
J'ai effectué une petite série de tests avec des valeurs de numéros de version simples et ça fonctionne bien. Il me reste à tester avec des trucs un poil plus exotiques pour vérifier que la bascule vers les méthodes suivantes se fait bien.

@valanc
Copy link
Author

valanc commented May 10, 2016

Attention, ce n'est pas encore tout bon !
Voici mon code :

def compare_versions(version1, version2):
    from distutils.version import StrictVersion, LooseVersion
    v1 = version1.encode('ascii', 'ignore')
    v2 = version2.encode('ascii', 'ignore')
    try:
        return cmp(StrictVersion(v1), StrictVersion(v2))
    except ValueError:
        # in case of abnormal version number, fall back to LooseVersion
        try:
            return cmp(LooseVersion(v1), LooseVersion(v2))
        except TypeError:
            # certain LooseVersion comparions raise due to unorderable types, fallback to string comparison
            return cmp([str(v) for v in LooseVersion(v1).version], [str(v) for v in LooseVersion(v2).version])

Il est nécessaire de convertir les chaînes Unicodes en string.

Pour le reste, il ne faut pas utiliser softtab, mais les numéros de versions contenus dans l'enregistrement (boucle for). De plus, c'est l'inverse au niveau des tests :

  1. lower
    Ne pas installer si la version installée est supérieure ou égale à la condition
    -> compare_versions(s.version, condition.softwareversion) >= 0
  2. higher
    Ne pas installer si la version installée est inférieure ou égale à la condition
    -> compare_versions(s.version, condition.softwareversion) <= 0
    Ce qui donne le code suivant :
    # Software not installed or version lower than (one wildcard can be used for condition name)
    if install == True:
        for condition in pack.conditions.filter(depends='lower'):
            if '*' in condition.softwarename:
                nametab = condition.softwarename.split('*')
                if software.objects.filter(host_id=m.id, name__startswith=nametab[0],name__endswith=nametab[1]).exists():
                    softtab = software.objects.filter(host_id=m.id, name__startswith=nametab[0],name__endswith=nametab[1])
                    for s in softtab:
                        if compare_versions(s.version, condition.softwareversion) >= 0:
                            install = False
                            status(''+str(m.id)+''+str(pack.id)+'Warning condition: '+escape(condition.name)+'')
                            break
            else:
                if software.objects.filter(host_id=m.id, name=condition.softwarename).exists():
                    softtab = software.objects.filter(host_id=m.id, name=condition.softwarename)
                    for s in softtab:
                        if compare_versions(s.version, condition.softwareversion) >= 0:
                            install = False
                            status(''+str(m.id)+''+str(pack.id)+'Warning condition: '+escape(condition.name)+'')
                            break
    # Software installed and version higher than
    if install == True:
        for condition in pack.conditions.filter(depends='higher'):
            if '*' in condition.softwarename:
                nametab = condition.softwarename.split('*')
                if software.objects.filter(host_id=m.id, name__startswith=nametab[0],name__endswith=nametab[1]).exists():
                    softtab = software.objects.filter(host_id=m.id, name__startswith=nametab[0],name__endswith=nametab[1])
                    for s in softtab:
                        if compare_versions(s.version, condition.softwareversion) <= 0:
                            install = False
                            status(''+str(m.id)+''+str(pack.id)+'Warning condition: '+escape(condition.name)+'')
                            break
                else:
                    install = False
                    status(''+str(m.id)+''+str(pack.id)+'Warning condition: '+escape(condition.name)+'')
            else:
                if software.objects.filter(host_id=m.id, name=condition.softwarename).exists():
                    softtab = software.objects.filter(host_id=m.id, name=condition.softwarename)
                    for s in softtab:
                        if compare_versions(s.version, condition.softwareversion) <= 0:
                            install = False
                            status(''+str(m.id)+''+str(pack.id)+'Warning condition: '+escape(condition.name)+'')
                            break
                else:
                    install = False
                    status(''+str(m.id)+''+str(pack.id)+'Warning condition: '+escape(condition.name)+'')

@dam09fr
Copy link

dam09fr commented May 10, 2016

En effet j'ai déposé le mauvais fichier ...
Je viens de m'en apercevoir ... j'ai remis le bon (j'aurais pas pu jouer un test avec ce que j'ai livré) !
Je n'ai pas intégré la conversion en ascii pour le moment (j'ai pas ce caractères spéciaux dans mes numéros de version ;-)
https://github.com/dam09fr/updatengine-server/tree/dam09fr-patch-versioncompare

dam09fr added a commit to dam09fr/updatengine-server that referenced this pull request May 10, 2016
@dam09fr
Copy link

dam09fr commented May 10, 2016

Voilà, j'ai pris en compte tes remarques. Qu'en pense-tu ?
https://github.com/dam09fr/updatengine-server/tree/dam09fr-patch-versioncompare

@valanc
Copy link
Author

valanc commented May 10, 2016

Parfait, ça semble correspondre au code qui fonctionne sur mon serveur UE !

Pour tester l'existence d'un logiciel (quelle que soit la version), je ne suis pas certain de bien m'y prendre :

  • Condition = Logiciel installé et version supérieure à :
  • Version = 0
    Dans ce cas, le 0 n'est pas supporté par StrictVersion.

Si j'ai bien compris, tu mets une condition du type "Logiciel / version est installé" avec le champ version vide !?

@dam09fr
Copy link

dam09fr commented May 10, 2016

En fait je teste si le logiciel n'est pas installé, du coup en laissant à vide la fonction renvoie True du moment ou le logiciel possède un numéro de version.

Je voulais créer un test : "logiciel est installé et version inférieure à", je suppose qu'on peut tout aussi bien créer un test "logiciel installé"

@valanc
Copy link
Author

valanc commented May 10, 2016

Je note que tu as finalement intégré la conversion ASCII ! ;-)
Moi non plus je n'ai pas de caractères si particuliers, mais la chaîne Unicode provoquait une erreur...

@dam09fr
Copy link

dam09fr commented May 10, 2016

Je te suis dans ta réflexion ! C'était logique, les classes ne supportaient pas l'unicode.
Concernant la pull request, que fait-on ?

@valanc
Copy link
Author

valanc commented May 10, 2016

J'avais demandé il y a longtemps une condition du type "Logiciel installé et version inférieure à :" pour faciliter la mise à jour de logiciels qui ne sont pas affectés à un profil. Une méthode de contournement m'a été indiquée sur le Forum. Il suffit de combiner 2 conditions :

  • Logiciel présent
  • Logiciel non installé ou version inférieure à
    Pour "Logiciel présent", je viens de tester : " Logiciel / version est installé" avec version vide ne fonctionne pas. Cependant, il ne me semble pas utile d'ajouter une condition spécifique qui ignorerait le numéro de version. On peut continuer à utiliser "Logiciel installé et version supérieure à :" avec version 0 (ou 0.0). A moins de corriger le code pour prendre en compte un numéro de version vide.

En revanche, la condition "Logiciel installé et version inférieure à :" pourrait simplifier les choses !

Concernant le Pull Request, je supprime ma demande et tu fais une demande ou je modifie ma demande ?

@dam09fr
Copy link

dam09fr commented May 10, 2016

Compte tenu que tu est l'initiateur et que la présente conversation est le cheminement de notre réflexion, je t'invite à modifier ta demande pour prendre en compte ce que nous avons à proposer.

Pour la condition, je vais regarder ce que je peux faire ...

@valanc
Copy link
Author

valanc commented May 11, 2016

Nouveau commit effectué.

Concernant les conditions, je n'ai pas bien compris ta méthode pour tester la présence ou non d'un logiciel...
Sur mon ordinateur, j'ai un logiciel XXX en version N :

  • La condition "Logiciel / version est installé" avec un numéro de version vide retourne False
  • La condition "Logiciel / version n'est pas installé" avec un numéro de version vide retourne False
    En revanche, on doit pouvoir tester la présence d'un logiciel par une condition du type "Logiciel installé et version supérieure à :" avec un numéro de version vide (plutôt que 0 comme je le faisais). A vérifier !
    A l'inverse, l'absence serait testé par une condition "Logiciel non installé ou version inférieure à :" avec le numéro de version "zzz"... Un peu sioux, mais à voir si c'est vraiment utile !

@dam09fr
Copy link

dam09fr commented May 16, 2016

Je viens de contrôler ton commit, tout est ok pour moi !
En attente de voir si l'auteur d'UE voudra bien l'intégrer ...

dam09fr added a commit to dam09fr/updatengine-server that referenced this pull request May 16, 2016
@dam09fr
Copy link

dam09fr commented Jan 31, 2017

Bien qu'ayant perdu tout espoir que cette modification soit un jour prise en compte, je poste ici une modification apportée afin de gérer le cas ou une condition d'installation "Logiciel non installé ou version inférieure à" ne contienne pas de numéro de version (champ vide) et qui générait une erreur bloquant tous les déploiements.

def compare_versions(version1, version2):
    from distutils.version import StrictVersion, LooseVersion
    if version1 == "" or version2 == "":
        return 0

@valanc
Copy link
Author

valanc commented Jan 31, 2017

Bonjour Damien,
Effectivement, c'est un peu désespérant... Autant pour la partie serveur on peut facilement s'en sortir, mais il serait temps de packager un nouveau client intégrant les modifications que tu proposes !
Concernant l'erreur que tu rapportes, il s'agit d'une condition mal formée (ça ne devrait pas être autorisé). Il me semble qu'en cas d'absence de numéro de version on est censé avoir la chaine de caractères "undefined".
Pour éviter de bloquer le système, je propose le code suivant :

     if version1 == "":
        v1 = 0
    else:
        v1 = version1.encode('ascii', 'ignore')
    if version2 == "":
        v2 = 0
    else:
        v2 = version2.encode('ascii', 'ignore')

Ainsi, un numéro de version absent est assimilé à 0 (le plus petit numéro) pour tenter une comparaison, plutôt que de supposer que c'est égal. Qu'en penses-tu ?

@valanc valanc closed this Jan 31, 2017
@valanc valanc reopened this Jan 31, 2017
@dam09fr
Copy link

dam09fr commented Jan 31, 2017

Bonjour,
L'idée me semble plutôt bonne en effet, je vais faire quelques tests avec ta proposition pour voir si tout fonctionne comme attendu.

En ce qui concerne le client, j'avais espéré que l'auteur me fournirai son script de compilation python et la source InnoSetup de l'installeur mais je n'ai jamais eu de réponse...
J'ai réussi avec succès à compiler le code python en exécutable distribuable en utilisant cx_freeze et j'ai réussi à décomposer une partie de l'installeur (mais pas les parties en pascal) ce qui fait que je serai bientôt en mesure de recomposer un installeur mais avec quelques contraintes : il faudra assigner manuellement les options (serveur, fréquence, certificat, ...) avant de construire l'installeur et on ne pourra pas les préciser pendant l'installation.
Si tu est intéressé on peut éventuellement entrer en contact pour que tu m'aide si tu t'en sens le courage ;-)

@dam09fr
Copy link

dam09fr commented Jan 31, 2017

Ah pour ce qui est de "undefined" dans le numéro de version, je n'ai absolument rien vu dans le code qui gère ça, la chaine de caractères doit être exploitée par le code comme étant un numéro de version si ma lecture est correcte.

@dam09fr
Copy link

dam09fr commented Jan 31, 2017

Je viens de tester ton code, j'ai les erreurs suivantes dans le log du client:

Traceback (most recent call last):
  File "updatengine-client.py", line 155, in main
...
UeReadResponse: <?xml version="1.0" encoding="UTF-8"?>
<Response><Import>Import ok</Import>Error when importing inventory: (<type 'exceptions.AttributeError'>, AttributeError("StrictVersion instance has no attribute 'version'",), <traceback object at 0x7fc1c2a08e60>)</Response>

@valanc
Copy link
Author

valanc commented Feb 2, 2017

Bonjour,
Hum, ça doit être v1 et v2 qui devraient être de type string...
Remplacer v1 = 0 par v1 = "0" et v2 = 0 par V2 = "0" !
Dis-moi si ça fonctionne.

@valanc
Copy link
Author

valanc commented Feb 2, 2017

Lors de l'inventaire, si le numéro de version d'un logiciel est absent, son champ Version est mis à "undefined". Ainsi, côté inventaire de logiciels, il ne devrait jamais y avoir la chaine vide (paramètre version1 de la fonction compare_versions).
Lors de la création d'une condition d'installation, par défaut le champ Version est mis à "undefined"... Cependant, il est effectivement possible de valider la condition avec le champ Version vide, ce qui ne me semble pas normal : d'après moi, c'est une condition mal formée ! Pour tester la présence d'un logiciel quelque soit sa version, utiliser la condition du type "Logiciel installé et version supérieure à :" avec le numéro de version 0 (chaine de caractère "0").
Faute de validation des conditions, j'admets qu'il est préférable de sécuriser le test avec ces quelques lignes de code supplémentaires (au moins pour le paramètre version2 de la fonction compare_versions).

@dam09fr
Copy link

dam09fr commented Feb 4, 2017

Je rejoins tes conclusions et je suis d'accord avec la correction proposée.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants