Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add debian package #43

Open
za3k opened this issue Sep 23, 2022 · 17 comments
Open

Add debian package #43

za3k opened this issue Sep 23, 2022 · 17 comments
Labels
enhancement New feature or request help wanted Extra attention is needed
Milestone

Comments

@za3k
Copy link
Owner

za3k commented Sep 23, 2022

No description provided.

@za3k za3k added the enhancement New feature or request label Sep 23, 2022
@za3k za3k added this to the v1.1 release milestone Sep 23, 2022
@za3k za3k added the help wanted Extra attention is needed label Sep 23, 2022
@za3k
Copy link
Owner Author

za3k commented Oct 9, 2022

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
Copy link
Contributor

anarcat commented Feb 27, 2023

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
Copy link
Contributor

anarcat commented Feb 27, 2023

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
Copy link
Owner Author

za3k commented Feb 27, 2023 via email

@anarcat
Copy link
Contributor

anarcat commented Feb 27, 2023 via email

@anarcat
Copy link
Contributor

anarcat commented Feb 27, 2023

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
Copy link
Owner Author

za3k commented Feb 27, 2023 via email

anarcat added a commit to anarcat/qr-backup that referenced this issue Feb 28, 2023
This is a more standard way of doing things.
@anarcat
Copy link
Contributor

anarcat commented Feb 28, 2023

done, #46

za3k added a commit that referenced this issue Feb 28, 2023
move debian/ to top-level (#43)
@za3k
Copy link
Owner Author

za3k commented Mar 3, 2023

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

@anarcat
Copy link
Contributor

anarcat commented Mar 3, 2023

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
Copy link
Contributor

anarcat commented Mar 3, 2023

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
Copy link
Owner Author

za3k commented Mar 3, 2023 via email

@anarcat
Copy link
Contributor

anarcat commented Mar 4, 2023 via email

@za3k
Copy link
Owner Author

za3k commented Aug 15, 2023

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

@za3k
Copy link
Owner Author

za3k commented Aug 15, 2023

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
Copy link
Contributor

anarcat commented Aug 15, 2023

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
Copy link
Owner Author

za3k commented Aug 15, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants