FFmpeg subprocess.Popen stdout leer?

[__deets__]

@harryberlin

Ergaenzend zu BlackJacks Anmerkungen: dein Vorgehen ist nicht robust gegenueber Aenderungen der Fehlertexte, sei es durch veraenderte Versionen (was natuerlich auch andere Probleme mit sich bringen kann), oder auch nur ein anderes locale, wodurch Fehlertexte in einer anderen Sprache ausgegeben werden koennen.

[BlackJack]

@harryberlin: Anmerkungen zum Code zusätzlich zu dem was Sirius3 schon gesagt hat:

Wenn man die Kommandozeilenargumente über ein Wörterbuch auf Funktionen abbildet, kann man die Meldung bei einem unbekannten Kommando leichter fehlerfrei hinbekommen, weil man die aus den Schlüsseln erstellen kann, statt das man immer daran denken muss bei Veränderungen bei den Kommandos auch diese Ausgabe entsprechend anzupassen.

`DEVICE_NAMES` wird nicht verwendet. `DIRCHECKER` ist keine Konstante und hat dort auf Modulebene auch nichts zu suchen. Dem Thread einen Namen zu geben, macht nicht wirklich Sinn.

Warum der Name `__icon__` in `note()`? Namen mit zwei führenden und folgenden Unterstrichen sind per Konvention für Werte vorgesehen die im Python-Ökosystem eine besondere Bedeutung haben, also entweder mit der Sprache oder mit Werkzeugen auf besondere Weise interagieren. Beispiele wären die ganzen ”magischen” Methoden die festlegen wie sich Objekte verhalten (Operatoren, ``for``, ``with``, …) oder `__doc__`, `__author__`, `__version__`, die von Hilfesystemen und Dokumentationsgeneratoren ausgewertet werden.

Da `busy_show()` und `busy_close()` immer in Paaren vorkommen, die Code ”umschliessen”, würde es sich anbieten da einen Contextmanager draus zu machen und ``with`` zu verwenden.

Wenn Du grundsätzlich die Ausgabe von externen Programmen protokollieren willst, dann bietet sich ja alleine deshalb schon eine Funktion dafür an. Damit man nicht jedes mal zusammen mit dem Aufruf noch einen `log()`-Aufruf schreiben muss.

Die `is_device_on()`-Funktion sieht sehr abenteuerlich aus. Erst einmal kann man den ``pass``-Zweig eleminieren. Dann sollte Ausnahmebehandlung präziser sein. Also weniger Code und nur die Ausnahme(n) umfassen, die man hier auch tatsächlich erwartet. Das ist also wohl das `os.listdir()` und eine `OSError`-Ausnahme, für den Fall das einem das Verzeichnis vor der Nase weggelöscht wird. Andererseits ist das `os.listdir()` in der Schleife die einzige Quelle für einen `OSError` und die Aktion ist das verlassen der Schleife. Also kann man die Ausnahmebehandlung auch um die Schleife legen, und damit den Sonderfall aus der Schleife herausziehen. Wenn man das gemacht hat, fällt auf, dass diese Ausnahmebehandlung dort nichts macht, also eigentlich nur dazu da ist das `disconnect()` davor zu schützen von einer Ausnahme übergangen zu werden. Also kann man ``try``/``finally`` verwenden.

Jetzt gilt es noch den gruseligen `counter` weg zu bekommen der nur Werte die Werte -1, 0, und 1 annehmen kann, aber eigentlich nur ein Wahrheitswert ist, der ungünstig ausgedrückt wurde. Denn eigentlich will man ja, dass die Schleife solange wartet, bis keine Dateien mehr in dem Pfad vorhanden sind. Das lässt sich *wesentlich* simpler ausdrücken.

Ich lande dann ungefähr hier (ungetestet):

Code: Alles auswählen

#!/usr/bin/env python
# -*- coding: utf-8 -*-
 
# sudo apt-get install jmtpfs
 
# sudo mkdir ~/Android
# sudo chmod 775 ~/Android
# sudo jmtpfs -o allow_other ~/Android
 
# sudo umount ~/Android
# sudo rmdir ~/Android

import os
import subprocess
import sys
import time
from contextlib import contextmanager
from threading import Thread

import xbmc
import xbmcaddon
import xbmcgui
 
__author__ = 'harryberlin'

ANDROID_PATH = os.path.expanduser('~/Android')
 
APT_UPDATE = ['sudo', 'apt-get', 'update']
INSTALL_JMTPFS = ['sudo', 'apt-get', 'install', '-y', 'jmtpfs']
REMOVE_JMTPFS = ['sudo', 'apt-get', 'remove', '-y', 'jmtpfs']
MAKE_DIR = ['sudo', 'mkdir', ANDROID_PATH]
REMOVE_DIR = ['sudo', 'rmdir', ANDROID_PATH]
MOUNT_DEVICE = ['sudo', 'jmtpfs', '-o', 'allow_other', ANDROID_PATH]
UNMOUNT_DEVICE = ['sudo', 'umount', ANDROID_PATH]
 
 
def log(message):
    xbmc.log('plugin.script.mtpconnect: %s' % message)


def notify(header, message=' ', display_duration=3000):
    xbmcgui.Dialog().notification(
        heading=header,
        message=message,
        icon=os.path.join(xbmcaddon.Addon().getAddonInfo('path') + 'icon.png'),
        time=display_duration,
    )
    log('NOTIFICATION: %s - %s' % (header, message))


@contextmanager
def busy_shown():
    xbmc.executebuiltin('ActivateWindow(busydialog)')
    try:
        yield
    finally:
        xbmc.executebuiltin('Dialog.Close(busydialog)')

 
def ask_confirmation(message1, message2=' ', message3=' '):
    return xbmcgui.Dialog().ok(
        heading='MTP Connect', line1=message1, line2=message2, line3=message3
    )


def open_settings():
    xbmcaddon.Addon().openSettings()


def call(command):
    try:
        try:
            output = subprocess.check_output(command, stderr=subprocess.STDOUT)
        except subprocess.CalledProcessError as error:
            output = error.output
            return error.returncode
        else:
            return 0
    finally:
        log(output)


def install():
    with busy_shown():
        log('update package libary')
        call(APT_UPDATE)
        log('install package and add mount-path')
        call(INSTALL_JMTPFS)
        call(MAKE_DIR)
    # 
    # FIXME This message is wrong if any of the calls above did not
    #   succeed.
    # 
    notify('Package installed', 'Path %s created' % ANDROID_PATH)
 
 
def deinstall():
    with busy_shown():
        log('remove package')
        call(REMOVE_JMTPFS)
        log('delete directory')
        call(REMOVE_DIR)
    # 
    # FIXME This message is wrong if any of the calls above did not
    #   succeed.
    # 
    notify('Package deinstalled', 'Path %s removed' % ANDROID_PATH)
 

def disconnect(show_notification=False):
    log('try to unmount')
    call(UNMOUNT_DEVICE)
    # 
    # FIXME This message may be wrong if any of the calls above did
    #   not succeed.
    # 
    if show_notification:
        notify('Android Device disconnected')

  
def wait_for_unmount():
    try:
        # 
        # FIXME Use a better indicator than an empty path.
        #   `os.path.ismount()` for instance.  Maybe even replace
        #   polling by `pyinotify` where available.
        # 
        while os.listdir(ANDROID_PATH):
            time.sleep(1)
    finally:
        disconnect(True)
 

def connect():
    disconnect()
    ask_confirmation(
        'Some Devices only share their Folders,',
        'when Screen is unlocked.',
        'UNLOCK YOUR DISPLAY'
    )
    log('try to mount')
    if call(MOUNT_DEVICE) == 0:
        notify('Android Device connected', 800)
        thread = Thread(target=wait_for_unmount)
        thread.daemon = True
        thread.start()
    else:
        notify("CAN'T CONNECT DEVICE")
 
 
def main():
    command_name2func = {
        'connect': connect,
        'deinstall': deinstall,
        'disconnect': disconnect,
        'install': install,
    }
    if len(sys.argv) > 1:
        command_name = sys.argv[1].partition(';')[0]
        command = command_name2func.get(command_name)
        if command:
            command()
        else:
            notify(
                'Unknown command given!',
                '{0} are available'.format(
                    ', '.join(sorted(command_name2func))
                ),
                5000
            )
    else:
        open_settings()


if __name__ == '__main__':
    main()

[harryberlin]

danke für die mühe. ich muss gestehen, ich bin etwas verärgert, der größte teil meines codes ist umgeschrieben. hat sozusagen den beigeschmack, dass alles mist war. sogar verständliche sachen hast du geändert. da wäre mir lieber gewesen, wenn wir mal kontakt gehabt hätten, um die gedankengänge auszutauschen, ggf. auch eine begründung zu nennen. da ich oft bei deinen umschreibenden erklärungen nicht mitkomme.
der code sind die anfänge und noch kein fertiges projekt, hätte ich vllt erwähnen sollen.

das wärs doch ungefähr gewesen. aber wozu das zweifache "try:"
log möchte ich evtl. gar nicht immer.

Code: Alles auswählen

def call(command):
    try:
        try:
            output = subprocess.check_output(command, stderr=subprocess.STDOUT)
        except subprocess.CalledProcessError as error:
            output = error.output
            return error.returncode
        else:
            return 0
    finally:
        log(output)

[BlackJack]

@harryberlin: Also in dem Code den Du gezeigt hattest, da wolltest Du immer `log()` aufrufen. Ich habe da halt nur das immer wiederkehrende Muster in eine Funktion herausgezogen.

Das zweite ``try`` ist da weil ich nicht genau wusste ob sich das ``finally`` auch auf den ``else``-Block beziehen würde. Tut es wahrscheinlich, aber so ist es auf jeden Fall eindeutig.

[Sirius3]

@BlackJack: Du wandelst eine Exception in einen Rückgabewert um???
Dann kann man sich das ganze ja gleich sparen:

Code: Alles auswählen

def call(command):
    process = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
    output, _ = process.communicate()
    return_code = process.wait()
    log(output)
    return return_code

[BlackJack]

@Sirius3: Dann müsste man es aber wieder umschreiben wenn man sich an die ganzen FIXME-Kommentare macht, denn da müsste man dann ja immer den Rückgabewert prüfen. Spätestens dann würde ich `call()` die Ausnahme nach dem loggen wieder mit ``raise`` zum Aufrufer weiterreichen.

[harryberlin]

OK, da mein Code wohl doch etwas mehr Diskussion bedarf, eröffne ich gleich nen eigenen Thread.
viewtopic.php?f=1&t=38532
BTW: Wisst ihr warum ich keine E-Mailbenachrichtigung erhalte?