za3k / qr-backup

Paper backup of files using QR codes
Other
132 stars 12 forks source link

Add debian package #43

Open za3k opened 2 years ago

za3k commented 2 years ago

Help is wanted here. I have a .deb built, but I would like someone else to take responsibility for actually uploading. Also, there's a missing dependency (python-reedsolo)

anarcat commented 1 year ago

Hi!

I found your project in Debian's "WNPP" archives:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1021089

There I asked if you needed help with this, before finding this issue, so I guess I'm here to propose our help!

Normally, when you file an "ITP" (Intent To Package) as you did, you intend to do the packaging yourself and it's unlikely someone will pick up that work unless you explicitly ask for help or it "times out" (and turns into an "RFP", a Request For Package). Nevermind, I misread the bug report, someone else volunteered and I will contact them first.

So I guess I'll look at packaging python-reedsolo next! :)

anarcat commented 1 year ago

So I guess I'll look at packaging python-reedsolo next! :)

and that's https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=985095 and i'm building a package now.

za3k commented 1 year ago

On 2023-02-27 11:21 am, anarcat wrote:

Hi!

I found your project in Debian's "WNPP" archives:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1021089

There I asked if you needed help with this, before finding this issue, so I guess I'm here to propose our help!

Normally, when you file an "ITP" (Intent To Package) as you did, you intend to do the packaging yourself and it's unlikely someone will pick up that work unless you explicitly ask for help or it "times out" (and turns into an "RFP", a Request For Package).

So I guess I'll look at packaging python-reedsolo next! :)

I tried to get a sponsor on #debian-python with no luck. Thanks for following up!

Both qr-backup and python-reedsolo are at https://germinate.za3k.com/pub/debian/, in addition to the source for qr-backup packages you already found.

anarcat commented 1 year ago

On 2023-02-27 12:45:49, Zachary Vance wrote:

I tried to get a sponsor on #debian-python with no luck. Thanks for following up!

Both qr-backup and python-reedsolo are at https://germinate.za3k.com/pub/debian/, in addition to the source for qr-backup packages you already found.

oh. bad luck there: i have repackaged reedsolo already! i have just uploaded it to the archive and it should land in NEW shortly.

please review the package here to see if it's up to what you would have expected:

https://salsa.debian.org/python-team/packages/reedsolo/-/tree/debian/master/

anarcat commented 1 year ago

also, about the qr-backup debian package itself... i see you have the source package stuff in package/debian... if you move that up one level, you'll have that package pretty much in the right shape already... it's something I'd have to do to fix the package before upload, any chance you could tweak that already? :)

za3k commented 1 year ago

Yes, but probably in a couple days. If you shoot me a PR I'll approve it tonight.

On 2023-02-27 12:52 pm, anarcat wrote:

also, about the qr-backup debian package itself... i see you have the source package stuff in package/debian... if you move that up one level, you'll have that package pretty much in the right shape already... it's something I'd have to do to fix the package before upload, any chance you could tweak that already? :)

-- Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. You are receiving this because you authored the thread.Message ID: @.***>

Links:

[1] https://github.com/za3k/qr-backup/issues/43#issuecomment-1447070928 [2] https://github.com/notifications/unsubscribe-auth/AAFRLUUPQ2KTDXL5TZQJNR3WZUHZFANCNFSM6AAAAAAQUH6YMM

anarcat commented 1 year ago

done, #46

za3k commented 1 year ago

This was a dead link when I tried it: https://salsa.debian.org/python-team/packages/reedsolo/-/tree/debian/master/

anarcat commented 1 year ago

how about https://salsa.debian.org/python-team/packages/python-reedsolo? someone had already done the packaging and had a better version, which we decided was better than mine. it's waiting in NEW now.

anarcat commented 1 year ago

i looked at the source code around here a little bit, and i think it generally looks sane, but there's lots of room for improvement.

one thing that could be improved is the manpage stuff. in docs/ you have qr-backup.1.man and MAN.txt, which is a little confusing. is the former generated by the latter? if so, that should probably be done at build time and qr-backup.1.man (ideally) removed from git. now i see there's a hidden thing to generate those, shouldn't that be in the makefile instead? and while i'm talking about the makefile, it seems to me this would be better served by a setup.py or setup.cfg or pyproject.toml or whatever it is the latest thing python packaging does. :)

i'm not sure what to do about the embedded font. why is it there? font-dejavu is already in debian and basically every linux distribution out there, it seems like dead weight... i'm not sure it's completely against policy but typically we try to avoid vendored code copies, i'm just not sure it applies to fonts.

we could repackage the source to remove it, but that's more work for debian and divergence from upstream which we rather avoid.

there's also two LICENSE files and the debian/copyright which seem redundant...

the qr-backup code itself is a little messy and hard to read, a little all over the place. the QR class looks like a missed opportunity to regroup a bunch of functions as class methods instead. i would use logging.debug almost everywhere you use logging.info and add a --debug flag. i would suggest using argparse. i would add type hints as well, as right now it's really hard to tell where data is coming from and whether scanning an unknown set of codes could cause security issues.

in the restore test, you pass the pasphrase to gpg directly, on the commandline, which is a security liability, as some other user on the same system could sniff the passphrase from the process list.

but really, none of this should be a blocker for debian packaging. the copyright file and embedded font stuff might get the package rejected, but maybe that's a bridge we can cross then...

i hope you don't mind the review: i typically do an audit of the source code before uploading to debian for security reasons and i hope it's useful for you. :)

za3k commented 1 year ago

On 2023-03-03 9:11 am, anarcat wrote:

i looked at the source code around here a little bit, and i think it generally looks sane, but there's lots of room for improvement. one thing that could be improved is the manpage stuff. in docs/ you have qr-backup.1.man and MAN.txt, which is a little confusing. is the former generated by the latter? if so, that should probably be done at build time and qr-backup.1.man (ideally) removed from git. now i see there's a hidden thing to generate those, shouldn't that be in the makefile instead? and while i'm talking about the makefile, it seems to me this would be better served by a setup.py or setup.cfg or pyproject.toml or whatever it is the latest thing python packaging does. :) The makefile was added only for debian packaging, basically.

i'm not sure what to do about the embedded font. why is it there? font-dejavu is already in debian and basically every linux distribution out there, it seems like dead weight... i'm not sure it's completely against policy but typically we try to avoid vendored code copies, i'm just not sure it applies to fonts. The font is not included in the built debian package, just the source. qr-backup is designed as much as possible to ship so it runs on a computer from 20 years ago with no special packages. (It falls short in many regards)

there's also two LICENSE files and the debian/copyright which seem redundant... debian/copyright being redundant seems like a problem debian has created :)

IIRC I packaged the debian stuff following the recommendations of the debian wiki. If you want to update that, it would be a public good, probably. You could mention "how to go from packaging it locally to having in be in debian" especially.

the qr-backup code itself is a little messy and hard to read, a little all over the place. the QR class looks like a missed opportunity to regroup a bunch of functions as class methods instead. i would use logging.debug almost everywhere you use logging.info and add a --debug flag. i would suggest using argparse. i would add type hints as well, as right now it's really hard to tell where data is coming from and whether scanning an unknown set of codes could cause security issues.

The qr-backup code IMO is hurt by being in one file, but we want that for good reasons. Type hints are a good idea, I've been avoiding them for silly reasons. Argparse would not make this code better. Renaming --verbose as --debug might be reasonable. IIRC The reason I don't use logging.debug is that one of the modules I'm using prints a BUNCH of garbage to debug. Which is a dumb reason, do you know how to not print that from imports? I guess I could use a non-default logger or something?

but really, none of this should be a blocker for debian packaging. the copyright file and embedded font stuff might get the package rejected, but maybe that's a bridge we can cross then...

i hope you don't mind the review: i typically do an audit of the source code before uploading to debian for security reasons and i hope it's useful for you. :)

This review doesn't tell me much I don't know, but I certainly don't mind it. Your other issues you opened have been very useful.

Thanks for your work on making qr-backup better and for helping package it. Sorry I'm a bit non-programmer-mode atm, I really do intend to get to the issues you raise.

Edit: Fixed horrible linebreaks

anarcat commented 1 year ago

On 2023-03-03 09:37:54, Zachary Vance wrote:

On 2023-03-03 9:11 am, anarcat wrote:

[...]

The makefile was added only for debian packaging, basically.

You really don't need a Makefile to make a Debian package. Debian is happy to package software build with setuptools, poetry, etc...

i'm not sure what to do about the embedded font. why is it there? font-dejavu is already in debian and basically every linux distribution out there, it seems like dead weight... i'm not sure it's completely against policy but typically we try to avoid vendored code copies, i'm just not sure it applies to fonts. The font is not included in the debian package, just the source. qr-backup is designed as much as possible to ship so it runs on a computer from 20 years ago with no special packages. (It falls short in many regards)

Yeah so the trick here is there are two types of Debian packge. What you refer to here is the .deb, the "binary" package. But Debian also ships "source" packages which will include the font unless the tar file is "repackaged", as I mentioned.

there's also two LICENSE files and the debian/copyright which seem redundant... debian/copyright being redundant seems like a problem debian has created :)

Right. So it really depends on how you want to do this. If you want to take care of the debian package, this may be a "native" package in which case it makes sense to use only one copyright file. But typically, we package stuff separately from upstream and manage a separate debian/copyright that has a specific, "machine readable" format that upstream don't necessarily want to follow.

In this case, you can remove this duplication, as an upstream, but you don't have to. What I was pointing out was mostly redundancy between LICENSE and LICENSE.md.

IIRC I packaged the debian stuff following the recommendations of the debian wiki. If you want to update that, it would be a public good, probably. You could mention "how to go from packaging it locally to having in be in debian" especially.

It's a very long story, but to keep it simple: Debian is a very old project and there are many ways of doing things. If you point me to a specific wiki page that you think led you astray, I'm happy to review and discuss it. :)

the qr-backup code itself is a little messy and hard to read, a little all over the place. the QR class looks like a missed opportunity to regroup a bunch of functions as class methods instead. i would use logging.debug almost everywhere you use logging.info and add a --debug flag. i would suggest using argparse. i would add type hints as well, as right now it's really hard to tell where data is coming from and whether scanning an unknown set of codes could cause security issues.

The qr-backup code IMO is hurt by being in one file, but we want that for good reasons.

Yeah, I don't think that's the biggest problem here.

Type hints are a good idea, I've been avoiding them for silly reasons.

Same here. I avoided them for a long time, but I started using mypy recently, and it's really a game changer.

Argparse would not make this code better.

Maybe not, but it would make easier for external contributors to grok your code. :)

Renaming --verbose as --debug might be reasonable. IIRC The reason I don't use logging.debug is that one of the modules I'm using prints a BUNCH of garbage to debug. Which is a dumb reason, do you know how to not print that from imports? I guess I could use a non-default logger or something?

I've faced that problem before and yes, there are ways. It really depends on how the library code is architectured. Ideally, libraries have their own logger that you can tweak levels for. Which library is it?

but really, none of this should be a blocker for debian packaging. the copyright file and embedded font stuff might get the package rejected, but maybe that's a bridge we can cross then...

i hope you don't mind the review: i typically do an audit of the source code before uploading to debian for security reasons and i hope it's useful for you. :)

Your suggestions have been useful in general. The review doesn't tell me much I don't know, but I certainly don't mind it.

Thanks for your work on making qr-backup better and for helping package it. Sorry I'm a bit non-programmer-mode atm, I really do intend to get to the issues you raise.

Hey it's alright. :) Better to put stuff out there and improve it incrementally than wait for perfection and never do anything!

-- If we do not do the impossible, we shall be faced with the unthinkable.

za3k commented 1 year ago

python-reedsolo works great. Thanks for packaging it, and sorry for the long delay in getting back to this.

za3k commented 1 year ago

On 2023-03-03 09:37:54, Zachary Vance wrote: On 2023-03-03 9:11 am, anarcat wrote: but really, none of this should be a blocker for debian packaging. the copyright file and embedded font stuff might get the package rejected, but maybe that's a bridge we can cross then...

Great, should we get started on that? I will take a look at the suggestions and get started on those, but like you say I don't think they should be a blocker.

Let me know if the embedded font becomes a problem.

It's a very long story, but to keep it simple: Debian is a very old project and there are many ways of doing things. If you point me to a specific wiki page that you think led you astray, I'm happy to review and discuss it. :)

https://wiki.debian.org/Packaging/Intro?action=show&redirect=IntroDebianPackaging

Why don't you just take a look, and see if there is anything you thing needs updated? I think I can be out of the loop on this one, since I've forgotten most of the context in the last 6 months.

Same here. I avoided them for a long time, but I started using mypy recently, and it's really a game changer.

Opposite problem here. I worked at Dropbox, and we were Guido's beta testing guinea pigs for mypy. I kept worrying that stuff I knew would be out of date from the "final" version that shipped.

anarcat commented 1 year ago

https://wiki.debian.org/Packaging/Intro?action=show&redirect=IntroDebianPackaging

that looks moderately reasonable.

Great, should we get started on that? I will take a look at the suggestions and get started on those, but like you say I don't think they should be a blocker.

i am a bit too busy right now, not sure i will have much cycles for this unfortunately.

za3k commented 1 year ago

Darn, very reasonable six months later though. Thanks for packaging python-reedsolo at least