GNOME Bugzilla – Bug 784938
Parallelize package search with apt-file
Last modified: 2020-11-12 08:07:07 UTC
Searching packages with apt-file is painfully slow while the search in conducted in one thread only in `jhbuild/utils/systeminstall.py`. I'd be nice if it was parallelized.
Created attachment 355570 [details] [review] patch
Review of attachment 355570 [details] [review]: Thanks! I have two questions about this patch: * It looks like self.parallel is always True. Why do you add this variable? Why do you retain the code in the non-parallel paths? How would these ever be reached? It looks like it could all be removed. * It looks like you define some functions inside other functions with no line breaks. I don't know enough about Python to know if this is idiomatic, but I suspect not? It would be nicer to add some line breaks. It would probably be nicest to move them outside of their parent functions.
> It looks like self.parallel is always True. Why do you add this variable? Why do you retain the code in the non-parallel paths? How would these ever be reached? It looks like it could all be removed. I figured that I wanted to allow this to be passed down as an option, but that's overkill and requires a test (afaik systeminstall isn't covered so far - not that it's easy to test). I'll remove it > It looks like you define some functions inside other functions with no line breaks. I don't know enough about Python to know if this is idiomatic, but I suspect not? It would be nicer to add some line breaks. It would probably be nicest to move them outside of their parent functions. Python only cares about indentation, not empty lines, I'll add them.
Created attachment 360502 [details] [review] patch
Created attachment 360503 [details] [review] patch I figured it might be worth to keep `parallel` as an option because it's a severe change to the API and users might not want it, so I added a `--no-parallel` flag to the CLI. Regarding the function position, I figured that keeping them at the level inside the function increases readability (that's always controversial, of course) because they only serve to share code between parallel and non-parallel execution right after they're declared. I added this info as a comment. I added empty lines around those functions.
Created attachment 360505 [details] [review] patch Sorry for the many updates, this is my first patch...
Created attachment 360506 [details] [review] patch Need to take into account calls to `SystemInstall.install` of other `SystemInstall` implementations.
I assume this was fixed by https://gitlab.gnome.org/GNOME/jhbuild/-/merge_requests/5 and related MRs