Re: CVS commit: pkgsrc/chat/matrix-synapse

[Greg Troxel <gdt%lexort.com@localhost>]

"Adam Ciarcinski" <adam%netbsd.org@localhost> writes:

> Module Name:  pkgsrc
> Committed By: adam
> Date:         Wed Jan 31 00:05:15 UTC 2024
>
> Modified Files:
>       pkgsrc/chat/matrix-synapse: Makefile distinfo
> Removed Files:
>       pkgsrc/chat/matrix-synapse/patches: patch-pyproject.toml
>
> Log Message:
> matrix-synapse: allow py-pydantic v2; provide correct DEPENDs; clean up

Did you ask js@ to review this change?  It seems major enough that it should have
been tested in production, which is what js@ and I have been doing
before committing updates.   I am guessing not, because I probably would
have been asked to look at it and/or try it too.

>  # Dependencies as defined by synapse's build system (in theory):
> -# \todo Go over poetry.lock

You dropped this comment.  But the commit message does not say that you
have rigorously gone over the upstream documented-in-build depends.  (It
has seemed messy.)

> +DEPENDS+=    ${PYPKGPREFIX}-cryptography>=3.4.7:../../security/py-cryptography

It seems this was missing, but it was js@'s intent to not require recent
cryptography.  That may not longer make sense as the package needs rust
anyway, but if you didn't check with MAINTAINER it's not ok to drop such
support (such as the patch to pyproject.toml).

> -DEPENDS+=    ${PYPKGPREFIX}-idna>=2.5:../../www/py-idna
> -DEPENDS+=    ${PYPKGPREFIX}-lxml>=3.5.0:../../textproc/py-lxml
> -DEPENDS+=    ${PYPKGPREFIX}-nacl>=1.2.1:../../security/py-nacl

These show up in the sources.  Why did you remove them?

> -DEPENDS+=    ${PYPKGPREFIX}-psycopg2>=2.7:../../databases/py-psycopg2

Why is this dropped?  synapse needs a postgres database and we as
maintainers have more or less given up on sqlite3.

> -TEST_DEPENDS+=       ${PYPKGPREFIX}-test-[0-9]*:../../devel/py-test
>  
> @@ -112,12 +109,12 @@ post-install:
>  # \todo Grok upstream's new test scheme and port to it.
>  # As of 1.98.0 all tests fail, most of them failing to import
>  # synapse.synapse_rust.
> -do-test:
> -     cd ${WRKSRC} && ${SETENV} ${MAKE_ENV} pytest-${PYVERSSUFFIX}
> +# Quick fix before running tests: rm -r ${WRKSRC}/synapse

This looks like an improvement, but:

  Is the package now running tests the way upstream wants tests run?  If
  so, should the todo comment be removed?

  Tests all fail, but with the "quick fix", things seem better:
    2166 failed, 616 passed, 292 skipped, 198 warnings, 4 errors in 199.39s (0:03:19)

  Do you understand what's going on?  It seems like it should be
  explained in a comment.