Bug 1215478 - Review Request: lets-encrypt - A free, automated certificate authority
Summary: Review Request: lets-encrypt - A free, automated certificate authority
Keywords:
Status: CLOSED DUPLICATE of bug 1287193
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nikos Mavrogiannopoulos
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 998103 1198473 1279191 1286324
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-04-26 23:16 UTC by Torrie Fischer
Modified: 2015-12-04 15:02 UTC (History)
38 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-12-01 17:50:31 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Torrie Fischer 2015-04-26 23:16:16 UTC
Spec URL: https://hackerbots.net/~tdfischer/rpm/lets-encrypt.spec
SRPM URL: https://hackerbots.net/~tdfischer/rpm/lets-encrypt-04fb4f1-1.fc20.src.rpm
Description: Let's Encrypt is a free, automated certificate authority that aims to lower the barriers to entry for encrypting all HTTP traffic on the internet.
Fedora Account System Username: tdfischer

Comment 1 Xavier Bachelot 2015-04-27 08:58:53 UTC
Hi,

Some comments on the spec :
- If you're not using %{python_sitearch} don't define it at all. Also, this is probably only needed on older Fedora or EPEL, as these macros are provided by the python-devel package. Adjust depending on what distro/releases you are targeting.
- Version looks like a git hash rather than a proper version. Maybe you should just use 0 as the version until upstream makes a proper release and move the git hash to the release tag.
- Summary is too long and should not re-state the name of the package.
- Fix License tag. The code seems to be mostly licensed under ASL, only part of it is MIT.
- Source0 must be a full URL or you need to provide a script that can regenerate the tarball from the SCM.
- Remove "Description:" from the start of the %description section.
- In the %install section, cleaning the build root is not needed anymore.
- In the %files section, there is no file marked as %doc. If that really is the case, remove the line, but I suspect you'd rather add some.
- No license file included.
- The changelog line is missing the version and release of the package.

Regards,
Xavier

Comment 2 Xavier Bachelot 2015-04-27 09:34:46 UTC
Also, missing BuildRequires on python-setuptools.

Comment 3 Torrie Fischer 2015-04-30 04:58:46 UTC
Specfile updated to address issues. New srpm: https://hackerbots.net/~tdfischer/rpm/lets-encrypt-0-1.git1d8281d.fc20.src.rpm

And the script for generating tarballs from git: https://github.com/tdfischer/lets-encrypt-preview/blob/rpm/build-git.sh

Comment 4 Zbigniew Jędrzejewski-Szmek 2015-05-10 16:25:27 UTC
Xavier, are you taking the review?

Some notes from fedora-review:
- Package contains BR: python2-devel or python3-devel
(https://fedoraproject.org/wiki/Packaging:Python#BuildRequires)

- If (and only if) the source package includes the text of the license(s)
  in its own file, then that file, containing the text of the license(s)
  for the package is included in %license.
  Note: License file LICENSE.txt is marked as %doc instead of %license
  See:
  http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text
- lets-encrypt.noarch: W: incoherent-version-in-changelog 0-1.git1d8281d.fc20 ['0-1.git1d8281d.fc23', '0-1.git1d8281d']
- lets-encrypt.noarch: W: invalid-license ASL

Please see https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing for a list.

Comment 5 Zbigniew Jędrzejewski-Szmek 2015-06-12 20:37:51 UTC
Torrie, can you fix the things from comment 4 and post an updated version?

Comment 6 Sandro Mathys 2015-08-18 22:39:43 UTC
Shouldn't this package be called letsencrypt (without the "-")? Both the binary and the Github project/repo are written without the dash.

Comment 7 Richard Körber 2015-08-19 12:32:16 UTC
Some dependencies seem to be missing. On a clean F22 Server installation, I had to install another package and some Python pips to make the tool start:

dnf install python-requests

pip install python2-pythondialog
pip install pyrfc3339
pip install parsedatetime
pip install ConfigArgParse
pip install argparse

I haven't found rpm packages for the pips.

Comment 8 Zbigniew Jędrzejewski-Szmek 2015-08-20 00:18:16 UTC
argparse is in python itself. Don't know about the others.

Comment 9 Nikos Mavrogiannopoulos 2015-09-11 09:24:42 UTC
I'll take the review. Please provide an updated package that addresses the issues mentioned above and I'll take up from there.

Comment 10 Jakub Warmuz 2015-10-24 10:04:08 UTC
Hey, Let's Encrypt Client dev over here. We would love to be RPM packaged! Can upstream help in any way? :)

Comment 11 Felix Schwarz 2015-10-28 00:18:29 UTC
I don't want to interfer with the usual packaging process, just a quick note that I tried to fix some of the mentioned issues. I'm not sure if I'll have enough time to fix everything so I uploaded my current state as-is: https://fschwarz.fedorapeople.org/2015/letsencrypt-0-1.20151028git524f46d.fc22.src.rpm

In comment 7 it was mentioned that some dependencies were missing. Assuming this is true I think this bug should be blocked on
- bug 1198473 (python-configargparse, not yet reviewed)
- bug 998103 (python-dialog needs to be updated, hopefully we can do so at least for f23)

python-parsedatetime is already in Fedora. As far as I can see pyRFC3339 is not yet packaged.

Also there is some official(?) packaging work going on for Debian which might provide some valuable input: https://github.com/letsencrypt/letsencrypt-debian
In particular I think we should run unit tests as part of the build process and think about having sub-packages if we can reduce the amount of required packages significantly.

Comment 12 Felix Schwarz 2015-10-29 00:05:58 UTC
Just fyi: I built some rpms which work for me(tm) but without proper linting + some todos left (see spec file). As I don't have time to maintain letsencrypt in Fedora I pushed my work to copr so it might help someone else who does (not sure if I managed to set the right option in copr, maybe the build will fail but you should be able to retrieve the src.rpms): https://copr.fedoraproject.org/coprs/fschwarz/letsencrypt/

"works for me" = I was able to retrieve a certificate from the production CA using manual mode auth

Comment 13 Jakub Warmuz 2015-10-29 06:43:31 UTC
I would like to point out that Let's Encrypt team strongly suggests using tarballs from PyPI and not git master. https://github.com/letsencrypt/letsencrypt/wiki/Packaging

Comment 14 Nikos Mavrogiannopoulos 2015-10-29 08:18:02 UTC
To add this in Fedora we certainly need a packager who is able to follow up with the package. I'm willing to review this, or any other review request if we have that.

Comment 15 Dan Callaghan 2015-11-04 11:00:10 UTC
Torrie, are you still able to maintain this package? If not, I would be willing to take it. I'm sure there's plenty of others here willing to help as well. :-)

Comment 16 Dan Callaghan 2015-11-04 11:05:33 UTC
I also just noticed that Itamar Reis Peixoto has recently published a COPR for letsencrypt, based on Torrie and Felix's spec:

https://copr.fedoraproject.org/coprs/itamarjp/letsencrypt/builds/

It also includes a package for python-pyrfc3339 which we don't have an open review request for yet.

Itamar, are you planning to file a review request for python-pyrfc3339?

Comment 17 Felix Schwarz 2015-11-04 11:21:24 UTC
(In reply to Nikos Mavrogiannopoulos from comment #14)
> To add this in Fedora we certainly need a packager who is able to follow up
> with the package. I'm willing to review this, or any other review request if
> we have that.

Thank you very much for that offer. I think you can start with bug 1198473 which should be easy enough as it's not Fabian's first package :-)

Comment 18 James Hogarth 2015-11-05 22:07:26 UTC
(In reply to Dan Callaghan from comment #15)
> Torrie, are you still able to maintain this package? If not, I would be
> willing to take it. I'm sure there's plenty of others here willing to help
> as well. :-)

Seeing as I plan to use this on some of my systems I'd be more than happy to take it if Torrie doesn't respond.

Hmm the python-pyrfc3339 as it stands in that COPR wouldn't be valid for review I'd say as it appear to rely on and grab python-pytz during build:

Searching for pytz
Reading https://pypi.python.org/simple/pytz/
Best match: pytz 2015.7
Downloading https://pypi.python.org/packages/2.7/p/pytz/pytz-2015.7-py2.7.egg#md5=f89213bbd728cb56e94ea4a4740737f3
Processing pytz-2015.7-py2.7.egg
Moving pytz-2015.7-py2.7.egg to /builddir/build/BUILD/python-pyrfc3339-0.2/python2/.eggs

However pytz is already in Fedora so should be fairly simple to patch and use the system one for the buildrequires I'd expect ...

I'll have a play with it this weekend and if I can get something I'm happy with, and it's not in for review yet, will submit it.

Comment 19 James Hogarth 2015-11-05 22:10:33 UTC
Actually that said it only uses pytz during %check not %build ... I'd suggest it should still use the system one rather than grab from pypi regardless though ... 

I'll put something together over the weekend if no one else has by the time I look at reviews again sunday night.

Comment 20 Paul Howarth 2015-11-06 08:30:14 UTC
It *must* use the system one; in koji there is no external network access so grabbing from pypi wouldn't work even if it wasn't explicitly forbidden by the packaging guidelines.

https://fedoraproject.org/wiki/Packaging:Guidelines#Build_time_network_access

Comment 21 James Hogarth 2015-11-06 08:54:14 UTC
(In reply to Paul Howarth from comment #20)
> It *must* use the system one; in koji there is no external network access so
> grabbing from pypi wouldn't work even if it wasn't explicitly forbidden by
> the packaging guidelines.
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Build_time_network_access

Indeed, that would be a bit of a hindrance ... I'll blame lack of sleep on my 8 week old making that guideline slip my mind ;)

Okay I've got something together that does *not* trigger any network access. 

I'll file a package review request for it shortly and then that can block this one.

Comment 22 James Hogarth 2015-11-08 14:35:40 UTC
python-pyrfc3339 is now up for review BZ#1279191 - I'll mark it blocking this one

Comment 23 Nick 2015-11-10 08:16:52 UTC
Great to see this progressing. I'd be keen to help out with the EPEL packages.

Comment 24 Nikos Mavrogiannopoulos 2015-11-16 13:40:04 UTC
Sorry for not being able to promptly follow up. I believe all of the dependencies are now in. Would it make sense to start a new review request on letencrypt (I see Torie is inactive), or someone else update this one?

Comment 25 Nikos Mavrogiannopoulos 2015-11-26 16:00:21 UTC
I'll get back to that once there is an active owner.

Comment 26 Itamar Reis Peixoto 2015-11-27 18:30:25 UTC
let wait a bit more if no reply we can take this and continue without him.

Comment 27 Zbigniew Jędrzejewski-Szmek 2015-11-27 18:35:33 UTC
I'd suggest that you continue. The OP hasn't responded since June, and he or she can always be a co-maintainer if he or she wants.

Comment 28 James Hogarth 2015-11-27 19:58:24 UTC
Well with my package now in rawhide I can pick this up too...

Have some free time tonight whilst my little one refuses to sleep so putting together an initial stab at something appropriate - check back in a few hours for first results.

Comment 29 James Hogarth 2015-11-27 20:55:55 UTC
As a side issue I'm personally not too keen with conflating this package with python-acme and building both as part of the letsencrypt srpm ... 

I'm of the opinion this should be a separate package request as well and another blocker on this...

The letsencrypt-apache and letsencrypt-nginx plugins do make sense to have as subpackages of this srpm however ...

My reasoning is that in principle python-acme could be used by other python applications/scripts and is effectively an independent entity to the letsencrypt client itself and thus should have it's own entry in bugzilla and be built separately, however the plugins only make sense in the context of letsencrypt itself ... 

Any other thoughts on this or does that make sense and pass as a sensible separation?

Comment 30 Felix Schwarz 2015-11-27 20:57:57 UTC
acme should be definitively a separate package. When I updated the src.rpm I wasn't aware of the pypi releases which are separate. For the server specific packages, well I don't have a strong opinion.

Comment 31 James Hogarth 2015-11-27 21:04:43 UTC
Okay let's work on that basis - if no one else has python-acme up by the time I get to that (say Sunday) I'll pop a package review request up for that too...

Comment 32 James Hogarth 2015-11-27 23:17:34 UTC
Okay a few hours was optimistic ... going down some dependency pain getting a clean acme before I can test a clean letsencrypt ...

It is en route - expect to see something Sunday evening for both

Comment 33 Matthew Miller 2015-12-01 13:33:05 UTC
This is awesome -- thanks everyone for working on this! I know it's off-topic for the review per se, but would anyone be interested in working on a Fedora Magazine article on using these tools on Fedora Server?

Comment 34 James Hogarth 2015-12-01 13:40:23 UTC
(In reply to Matthew Miller from comment #33)
> This is awesome -- thanks everyone for working on this! I know it's
> off-topic for the review per se, but would anyone be interested in working
> on a Fedora Magazine article on using these tools on Fedora Server?

Noise in the review ticket! Bad Matt!

That's no problem - happy to do an article on that...

As a quick update python-acme had a few things to revise to get packaged - should be nearly there.

Then assuming no pickup from Torrie (it has been a while at this point) this can be finalised.

If there are no complaints I'm happy to open the new package review obsoleting this one... or I'm happy to be a co-maintainer with someone else opening the final review (we really should do that and close this duplicate at that time so the right requestor is in place for the person actually submitting the spec/srpm).

Comment 35 Itamar Reis Peixoto 2015-12-01 14:08:59 UTC
(In reply to James Hogarth from comment #34)
> If there are no complaints I'm happy to open the new package review
> obsoleting this one... 

ok, do it.

Comment 36 Matthew Miller 2015-12-01 15:29:15 UTC
> ok, do it.

+1 -- Torrie, if you're still able to help, add yourself as a co-maintainer to James' (upcoming) new ticket.

Comment 37 Matthew Miller 2015-12-01 15:33:39 UTC
PS: as per comment #6 and others, I think the new request should be "letsencrypt", no hyphen.

Comment 38 James Hogarth 2015-12-01 15:41:42 UTC
Found a few more runtime dependencies than described in #c7 ... going through something on my laptop to get something actually reviewable (right now it cannot pass review)...

It's getting very close to sane - I should have it finalised in a few days and will open a package review with a working spec/srpm then ...

There will still be a lag until fedora-review can run until python-acme is in rawhide though... and yes that'll be letsencrypt and not lets-encrypt for the package.

Comment 39 James Hogarth 2015-12-01 17:50:31 UTC
(In reply to Itamar Reis Peixoto from comment #35)
> (In reply to James Hogarth from comment #34)
> > If there are no complaints I'm happy to open the new package review
> > obsoleting this one... 
> 
> ok, do it.

Done - new bug is BZ#1287193 ... lets track updates to the spec from there.

*** This bug has been marked as a duplicate of bug 1287193 ***


Note You need to log in before you can comment on or make changes to this bug.