Closed juga0 closed 7 years ago
Thanks for the PR! I can't use the github review interface for this because everything is added anew (a result from the copy-operation you did) and thus i have no diffs to comment on here. It seems i can locally just copy your acpgpy/* files over to the autocrypt/ dir and look at the diffs but then i can't comment here on it, either. So could you do this copy operation yourself and remove acpgpy? This way we can discuss the diff here ... and is it right that the core of your work is currently in acpgpy/crypto.py?
The gpg backend needs to stay for the time being because I, and i think others, need integration into their existing gpg state -- i am fine to consider to make "bingpg" only an option but I won't through out this code and rely solely on pgpy in the next couple of months.
I am preparing and leaving for travel in a few hours until sunday and will try to then get back to the PR on monday -- please update it as mentioned before. I do not consider it crucial to get pgpy merged before easterhegg btw -- the session there i sketched as an overview of email encryption, some autocrypt approaches, probably abstract UI demos, and then a hands-on session where people try to get it to work if they have a suitable setting (most notably a local postfix instance which configures the autocrypt milter). I want to get some releases out which people can pip-install which also needs to happen and be tested early next week. And trying/refining the UI demos is obviously a priority if we want to use them in the session ...
That said, let's get pgpy into the core -- i'd like to have it and am happy to try it as default backend. It's great that you went through the effort of persevering in making it work ... it'd be very nice to have a self-contained python package which can do autocrypt and does not depend on random versions of GPG.
So could you do this copy operation yourself and remove acpgpy?
I have done so and push -f
so this PR is now based on that.
and is it right that the core of your work is currently in acpgpy/crypto.py?
yes, now called autocrypt/crypto.py.
The gpg backend needs to stay for the time being
that is fine also for me.
I do not consider it crucial to get pgpy merged before easterhegg
it would be nice though, as people could then contribute to that version.
I want to get some releases out which people can pip-install which also needs to happen and be tested early next week.
yeah, that would be nice too, it would be even better if that pip-install would include the pgpy version, but i know we have limited time.
the UI demos is obviously a priority if we want to use them in the session
agree.
That said, let's get pgpy into the core -- i'd like to have it and am happy to try it as default backend. It's great that you went through the effort of persevering in making it work ... it'd be very nice to have a self-contained python package which can do autocrypt and does not depend on random versions of GPG.
great, thank you.
So i have made a number of inline comments and have a few meta comments:
please remove logger.debug invocations almost completely, they make it harder to read the code and mostly seem like having been added for development
you removed bingpg.py and related command line options and handling but it needs to stay. My original suggestion (a few weeks ago) was to copy "bingpg.py" to "pgpy.py" (which you call "crypto.py"), and then make modifications to the other code to call into pgpy/crypto code instead of bingpg. I offered to then help to properly make pgpy an option. Your approach rather followed the idea to replace bingpg related code which now makes it harder for me to make pgpy just an option. Your approach also makes the PR much bigger than it otherwise needed to be ...
i am confused by the way how crypto.py handles/stores key information in the file system -- i'd think that only create and import of keys would create a file (maybe using the fingerprint as filename or so) and all other operations would just read from them. Also i am not sure yet i understand why you needed to add config.own_key
-- maybe it makes sense but why is a keyhandle not enough?
lastly your PR contains unrelated changes to metadata (in setup.py, requirements, coverage etc.) which i'd like to keep separate from adding pgpy support. Again makes the PR much larger.
Regarding to your comments to the comments, metadata and meta-comments:
Regarding to the comments i was (wrongly) enthusiastic to get:
This PR can:
I squashed commits so all this version is in only one.
Some tests are failing in python3 (probably just str/unicode) and some need to be reviewed because they might not have sense with the PGPy version (marked with TODO or FIXME).
To see the differences between the two implementations:
diff core/autocrypt acpgpy/autocrypt
.@hpk42, @dkg, opinions?