tech-pkg archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: import wip/prosody



Tiago Seco <tiago%seco.ws@localhost> writes:

> I've updated prosody to 0.11.3 and would like to ask for a review and
> import into pkgsrc.

Thanks for doing that.  My comments below should not be taken as
negative.  They are all really minor, which conveys that this is 99%
right.

A few minor things:

  0.11.4 is out

  The patch about hmac->myhmac has lost the comment.  Other than that,
  it looks like it was just regenerated.  (It's fine to regen but not to
  lose comments.)

  Not fair for a line number regen, but we have a notion that patches
  should be submitted (or a bug report filed) upstream, when
  appropriate, with the URL in the patch comments.  So if you are
  willing, that would be appreciated.

  -USE_LANGUAGES+=                c c99
  +USE_LANGUAGES=         c99
  I thought we have a notion that USE_LANGUAGES should always be +=, but
  reading the docs I see that this is not required and what you did is
  arguably better to remove the default of c.

  When commiting an update, it is required to have a commit message with
  what upstream should have put in NEWS (or what they did, even better).
  When I commit updates from wip, I like this to be in some COMMIT_MSG
  file already written, even with a "Update prepared in wip by X Y.", so
  after reviewing, the update commit itself is mechanical.

Things you didn't change and thus it isn't fair to demand, but:

  Not your change, so not reequired, but there is spurious extra newline
  in the config file patch, and it would be nice to have a comment
  explaining the intent of the changes.  (I do not object to the
  changes; I just like more comments than many others.)

  MASTER_SITES and HOMEPAGE are http, not https.  This is unlikely to be
  still correct.

  DESCR doesn't explain whether this is a just-barely or a
  mostly-complete implementation.  Reading the XEP list, it seems many
  are supported, and I don't know how to say that compactly.  Presumably
  TLS is supported, and one can configure a server to be TLS only.  But
  overall, I guess the point is that it's a valid choice for production,
  and that's hard to figure out.


Home | Main Index | Thread Index | Old Index