Linux Source Package Downloader

[__LC__]

Hallo liebe Python-Gemeinde,

weil ich mich zur Zeit mit der Interaktion zwischen Python und dem Betriebssystem (in meinem Falle Linux) auseinandersetze, habe ich mir mal ein kleines Programm geschrieben welches die Quellen von installierten Softwarepaketen herunterladen soll. Ich möchte das hier gern mal vorstellen und mir eure Meinung zur Umsetzung einholen, sprich was kann aus eurer Sicht wo verbessert werden. Schwerpunkt aus meiner Sicht läge besonders auf der Fehlerbehandlung, da ich hier nicht ganz auf einen grünen Zweig komme. Würde mich über Anregungen (auch kritischer Natur) sehr freuen.

Code: Alles auswählen

#!/usr/bin/python3

import os
import sys
import time
from optparse import OptionParser

def read_package_file(file):
	with open(file, "r") as file_object:
		return build_package_list(file_object)

def query_package_list():
	return build_package_list(os.popen("dpkg -l"))

def build_package_list(data_object):
	package_list = [line.split()[1] for line in data_object if line[:2] == "ii"]
	return package_list

def download_package(package):
	base_directory = os.getcwd()
	package_directory = "{0}/{1}".format(base_directory, package)
	package_status = True
	try:
		# create a folder for the package source
		path_exists = os.path.isdir(package_directory)
		if not path_exists: os.mkdir(package_directory)
		# go into the package folder
		os.chdir(package_directory)
		# download the sources -> no error == 0
		package_status = bool(os.system("apt-get source -s {0}".format(package)))
	except:
		pass
	finally:
		os.chdir(base_directory)
		# remove directory in case of an error and created by us
		if package_status == True and path_exists == False: os.system("rm -r {0}".format(package_directory))
		return package_status

if __name__ == "__main__":

	parameter_parser = OptionParser()
	parameter_parser.add_option("-b", "--basedir", dest="base_directory")
	parameter_parser.add_option("-f", "--file", dest="package_file")
	start_parameters = parameter_parser.parse_args()[0]

	if start_parameters.base_directory:
		try:
			os.chdir(start_parameters.base_directory)
		except OSError:
			# otherwise trigger an exit
			sys.exit("Invalid path")

	if start_parameters.package_file:
		try:
			package_list = read_package_file(start_parameters.package_file)
		except IOError:
			# something goes wrong here, so we leave the program immediately
			sys.exit("Error in reading the package file")
	else:
		package_list = query_package_list()

	failed_package_downloads = set()

	for package in package_list:
		if download_package(package):
			failed_package_downloads.add(package)

	# convert set -> string
	failed_package_downloads = ",".join(failed_package_downloads)
	sys.exit("Could not download sources from following packages: {0}".format(failed_package_downloads))

Schöne Grüße und vielen Dank!

[BlackJack]

@[LC]: Statt mit externen Programmen zu hantieren, sollte man vielleicht lieber die Python-Anbindung an die Debian-Paketverwaltung verwenden.

Wenn externe Programme, dann nicht mit `os.popen()` und schon gar nicht mit `os.system()` sondern mit dem `subprocess`-Modul.

Pfade nicht mit Zeichenkettenformatierung, sondern mit `os.path.join()` zusammen setzen.

`os.chdir()` vermeiden. Das verändert einen globalen Zustand im Programm, und wenn irgendwo die Fehlerbehandlung nicht das tut was man erwartet, oder auch nur ganz normaler Code, dann werden plötzlich Sachen in Verzeichnissen ausgeführt, mit denen man überhaupt nicht gerechnet hat.

Ein nacktes ``except:`` ohne konkrete Ausnahmen, die man erwartet ist böse. Noch schlimmer wenn dann kommentarlos einfach *gar nichts* gemacht wird. Damit verschluckt man an solchen Stellen *alle* Fehler, auch solche mit denen man nicht gerechnet hat. Und auch solche die man dann später sucht und nur ganz schwer findet, weil einem Null Hinweis darauf gegeben werden kann was und wo etwas schief läuft.

Vergleiche mit literalen Wahrheistwerten sind überflüssig und schlechter Stil. Man hat einen Wahrheitswert und vergleicht den mit einem anderen Wahrheitswert, und bekommt *Trommelwirbel* wieder einen Wahrheitswert. Damit hat man doch aber schon mal angefangen, also hätte man den auch gleich nehmen können. Ausnahme: Man möchte genau das Gegenteil wissen — dafür gibt es ``not``.

Bei den Optionen könnte man über sinvolle Defaultwerte nachdenken und so vielleicht ``if``\s einsparen.

Bei `failed_package_downloads` könnte man einen Generatorausdruck verwenden.

Die letzte Meldung wird auch ausgegeben, wenn keine Fehler aufgetreten sind.

Einrücktiefe ist per Konvention vier Leerzeichen — keine Tabs.

[__LC__]

Moin BlackJack,

vielen Dank für deine zahlreichen Anmerkungen. Ich werde mich dem heute Abend mal in Ruhe annehmen und durch deine angesprochenen Punkte durcharbeiten. Mir war überhaupt nicht bewusst dass es eine Anbindung an die Debian-Paketverwaltung gibt. Werde ich mir mal anschauen. Viel interessanter finde ich aber deine Anmerkung hinsichtlich des subprocess-Moduls. Kannst du kurz darlegen, was der entscheidende Unterschied zum os.popen() bzw. os.system() ist? Bzw. was sind die Nachteile letztgenannter?

Schöne Grüße

[BlackJack]

@[LC]: `os.popen()` und `os.system()` sind nur eine ganz dünne Schicht zu den gleichnamigen C-Funktionen. Insbesondere vom `system()`-Aufruf sollte man auch in C die Finger lassen. Beide führen eine Zeichenkette in einer extern gestarteten Shell aus. Das heisst die Zeichenkette muss alle Sonderzeichen für die Shell irgendwie schützen. Zu solchen Sonderzeichen gehören unter anderem so harmlose Sachen wie Leerzeichen in Argumenten, beispielsweise Pfadnamen. Da man nicht sicher weiss welche Shell das sein wird, kann das in Randfällen problematisch werden, weil man gar nicht sicher weiss was alles besonders behandelt werdern muss und wie. Bei `os.system()` weiss man beim Rückgabewert nicht ob der vom Programm kommt, welches man starten will oder von der Shell selbst, die unnötigerweise zwischen dem eigenen und dem gestarteten Programm läuft.

Eins ist mir noch aufgefallen: ``rm`` kann man auch in Python lösen.

[daemonTutorials]

@[LC]:
Eins ist mir noch aufgefallen: ``rm`` kann man auch in Python lösen.

Die Funktion dazu wäre os.remove()

Folgende Zeile könntest du, wie BlackJack bereits sagte, so abändern. Denn eine IF-Schleife fragt ja "WENN [Bedingung] = WAHR, DANN:" Also reicht es die Variablen hinzuschreiben:

Code: Alles auswählen

if package_status and not path_exists:

das heißt soviel wie "WENN package_status = WAHR AND path_exists = UNWAHR, DANN:". Dein Code fragt so: "WENN [package_status = WAHR] = WAHR AND [path_exists = UNWAHR] = WAHR, DANN:"

Übrigens die Doku sagt das selbe wie BlackJack:

On Unix, the return value is the exit status of the process encoded in the format specified for wait(). Note that POSIX does not specify the meaning of the return value of the C system() function, so the return value of the Python function is system-dependent.

Und hier ein Tipp aus Doku, wie man os.system() durch eine Funktion aus dem subprocess-Modul ersetzen kann: http://docs.python.org/3.3/library/subp ... -os-system

Ich hoffe, ich konnte das ganze noch mal verdeutlichen und kurz und bündig darstellen.

[lunar]

@daemonTutorials: http://if-schleife.de/