Bug 275167 - security/py-service-identity: merge duplicate of security/py-service_identity
Summary: security/py-service-identity: merge duplicate of security/py-service_identity
Status: Open
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: Po-Chuan Hsieh
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-11-18 11:29 UTC by Joakim Bergman
Modified: 2024-02-07 14:56 UTC (History)
5 users (show)

See Also:


Attachments
patch that merges the two ports (7.78 KB, patch)
2023-11-18 11:30 UTC, Joakim Bergman
no flags Details | Diff
[patch] deprecate older duplicate of py-service-identity (701 bytes, patch)
2023-11-20 08:48 UTC, John Hein
Axel.Rau: maintainer-approval+
Details | Diff
Remove duplicate port (16.74 KB, text/plain)
2023-11-20 09:03 UTC, Palle Girgensohn
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joakim Bergman 2023-11-18 11:29:27 UTC
security/py-service-identity introduced in https://cgit.freebsd.org/ports/commit/?id=96195f8763c81d6be29f0f00551c10d5904f545a seems to be a duplicate of security/py-service_identity albeit with a newer version and minor build differences.
Comment 1 Joakim Bergman 2023-11-18 11:30:47 UTC
Created attachment 246387 [details]
patch that merges the two ports
Comment 2 Joakim Bergman 2023-11-18 11:33:14 UTC
Should have opened with this, but the impact of the two ports existing is that on my system both were required by different ports. As they conflicted, the older was rejected and thus anything depending on the older was removed (py-twisted was the main one in my case).
Comment 3 Joakim Bergman 2023-11-18 11:38:12 UTC
Lastly, I updated maintainer to last committer (and maintainer of the duplicate port). It is not an endorsement of anyone as maintainer, I will leave that to Axel.Rau@Chaos1.DE and sunpoet@FreeBSD.org to decide.
Comment 4 Palle Girgensohn freebsd_committer freebsd_triage 2023-11-18 16:08:13 UTC
Hej Joakim!

Great! I just stumbled on this as well, since pkg-fallout nags me about my port www/web12ldap, that does not build at all for exactly this reason.

May I suggest using the latest port, or at least let the port's name be the same as upstreams. It is named service-identity nowadays. The `_' is replaced with `-'. It is also much more popular to use `-' as spacing character in python module names:

$ ls  -ld */py-*-* | wc -l
    1357
$ ls  -ld */py-*_* | wc -l    
      66


Albeit, there are ~15 depedant ports for service_identity and just one for service-identity, so I understand the rationale behind the suggest patch. I still think it would be worth to migrate to the same as upstreams. For one thing, the reason for the duplicate is probably just that the name changed upstreams.
Comment 5 Palle Girgensohn freebsd_committer freebsd_triage 2023-11-18 16:08:24 UTC
Hej Joakim!

Great! I just stumbled on this as well, since pkg-fallout nags me about my port www/web12ldap, that does not build at all for exactly this reason.

May I suggest using the latest port, or at least let the port's name be the same as upstreams. It is named service-identity nowadays. The `_' is replaced with `-'. It is also much more popular to use `-' as spacing character in python module names:

$ ls  -ld */py-*-* | wc -l
    1357
$ ls  -ld */py-*_* | wc -l    
      66


Albeit, there are ~15 depedant ports for service_identity and just one for service-identity, so I understand the rationale behind the suggest patch. I still think it would be worth to migrate to the same as upstreams. For one thing, the reason for the duplicate is probably just that the name changed upstreams.
Comment 6 Palle Girgensohn freebsd_committer freebsd_triage 2023-11-18 16:16:02 UTC
(In reply to Palle Girgensohn from comment #5)
Umm, OK, I guess that's is perhaps not true, since it installs as service_identity... NVM. I'll let the maintainers decide.
Comment 7 John Hein 2023-11-20 05:45:14 UTC
Upstream access to the name with the underscore redirects to the name with the hyphen.  Try https://github.com/pyca/service_identity

__init__.py in the source says the title of the package is service-identity.  Also python porting policy prefers that the name match PyPI (https://pypi.org/project/service-identity/).

I would be inclined to add DEPRECATED and EXPIRATION_DATE (set to, say, two months from now) to security/py-service_identity and give maintainers of ports that use the older port (with the underscore, now at 18.1.0) time to verify their ports do work with 23.1.0 (current version of py-service-identity).  This is probably something the committer who added py-service-identity should have done.

Why deprecate the underscore version?  Because it seems the port name with the hyphen agrees better with upstream.  And the older port (security/py-service_identity at 18.1.0) hasn't been keeping up with updates (no update to the port version since 2019 despite upstream releases since then).

I built devel/py-twisted with py-service-identity (now at ver 23.1.0).  It built fine with both the old and new version of service-identity, and 'make test' in py-twisted worked just as well with 18.1.0 as 23.1.0.
Comment 8 John Hein 2023-11-20 08:30:03 UTC
(In reply to Palle Girgensohn from comment #6)
The package indeed installs to site-packages/service_identity.  Installing to site-packages/service-identity breaks when you do 'import service-identity' since identifier names cannot include hyphens.  There are ways to import modules if the directory name has hyphens, but it's better if you don't do so.

PyPI package names are another story. You can use hyphens there although some style guides recommend just using alphabetic lowercase (see 'Package and Module Names' PEP 8). There are lots of package names that use uppercase and hyphens at pypi.org that do not follow that guideline, however - see https://packaging.python.org/en/latest/specifications/name-normalization/ (which allows underscores and hyphens and uppercase).  Maybe I am misreading PEP 8 (specifically what is considered a "package name").  But note that after the "name normalization" described in the 'Python Packaging User Guide', names with hyphens and underscores are considered equal in certain contexts (e.g., when provided to importlib.metadata functions).
Comment 9 John Hein 2023-11-20 08:32:43 UTC
(In reply to John Hein from comment #7)
"Also python porting policy prefers ..."

I meant specifically FreeBSD's python porting policy (https://wiki.freebsd.org/Python/PortsPolicy).
Comment 10 John Hein 2023-11-20 08:48:06 UTC
Created attachment 246437 [details]
[patch] deprecate older duplicate of py-service-identity

I would recommend this patch (and change the title of the bug to reflect the deprecation action).

Then ports that use security/py-service_identity can verify then can update to the newer version.
Comment 11 Palle Girgensohn freebsd_committer freebsd_triage 2023-11-20 08:59:58 UTC
(In reply to John Hein from comment #7)
(In reply to John Hein from comment #7)

If we deprecated either port, we anyway need to move all dependencies to the one we want to keep. Also att a CONFLICTS since the install the same service_identity file.

As it is now, ports are broken since their dependency chains scoop up both variants of the service[_-]identity ports.

Hence, I disagree. We can't let the tree be broken just like that?

Example is www/web2ldap (this is from a pkg-fallout email):

...
[main-amd64-default-job-04] |   `-- Installing py39-service_identity-18.1.0...
pkg-static: py39-service_identity-18.1.0 conflicts with py39-service-identity-23.1.0 (installs files into the same place).  Problematic file: /usr/local/lib/python3.9/site-packages/service_identity/__init__.py

Failed to install the following 1 package(s): /packages/All/py39-prometheus-client-0.17.1.pkg
=====
Message from py39-Automat-20.2.0:

--
Install graphics/py-graphviz and devel/py-twisted to enable state
machine visualization (`MethodicalMachine.asDigraph`).
*** Error code 1

Stop.
make: stopped in /usr/ports/www/web2ldap
Comment 12 Palle Girgensohn freebsd_committer freebsd_triage 2023-11-20 09:03:24 UTC
Created attachment 246439 [details]
Remove duplicate port

My suggestion removes the duplicate port and hence fixes the broken tree.
Comment 13 Axel.Rau 2023-11-20 10:16:35 UTC
Comment on attachment 246437 [details]
[patch] deprecate older duplicate of py-service-identity

I agree to deprecate service_identity 18.1.0
Comment 14 Palle Girgensohn freebsd_committer freebsd_triage 2023-11-20 12:18:33 UTC
(In reply to Axel.Rau from comment #13)
But that will not fix the current problem?

IF we deprecate it, dependant port that scoops up both ports via different dependencies will break.

Also, they must both get CONFLICTS since they install the same file.

I see no point in deprecating? It should be removed? Please explain the rationale around keeping the tree broken until January?
Comment 15 John Hein 2023-11-20 13:18:33 UTC
(In reply to Palle Girgensohn from comment #14)
No, it does not fix the situation where one port has a dependency list including py-service_identity and another port has a dependency list including py-service-identity.

But the original patch updates the port to 23.1.0 without considering whether dependent ports can / will work with the newer version.

Each port needs to be evaluated whether the newer version is acceptable (build and run-time) for that particular port.

See also bug 275207 for a py-twisted update to the new port.  The other ports that currently list the older py-service_identity (18.1.0) need similar consideration.
Comment 16 Po-Chuan Hsieh freebsd_committer freebsd_triage 2023-11-20 15:37:37 UTC
1. I'm busy recently. I'll take care of it this weekend.

2. We should use the name on PyPI which is "service-identity".
Comment 17 Palle Girgensohn freebsd_committer freebsd_triage 2023-11-20 16:43:56 UTC
(In reply to John Hein from comment #15)
regarding py-twisted, it's in the patch "Remove duplicate port" above, as are all ohter ports depending on the old version. I dropped the same patch in Phabricator for greater visibility [1].

Traditionally, we allow upgrading individual packages without every single maintainer for dependant ports being alerted and given three months headsup. This would be an anomaly.

The suggested patch [1] is tested with all relevant ports and builds fine. I have not tried running it though. The change list is at [2].

[1] https://reviews.freebsd.org/D42681

[2] https://service-identity.readthedocs.io/en/stable/changelog.html
Comment 18 commit-hook freebsd_committer freebsd_triage 2023-11-27 05:38:31 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=9f1e799a0b1209bbfe98e1da627ff4039ebe70fb

commit 9f1e799a0b1209bbfe98e1da627ff4039ebe70fb
Author:     Po-Chuan Hsieh <sunpoet@FreeBSD.org>
AuthorDate: 2023-11-27 05:38:03 +0000
Commit:     Po-Chuan Hsieh <sunpoet@FreeBSD.org>
CommitDate: 2023-11-27 05:38:03 +0000

    *: Change {RUN,TEST}_DEPENDS from security/py-service_identity to security/py-service-identity

    - Bump PORTREVISION for dependency change (except security/py-trustme)

    PR:             275167
    Reported by:    Joakim Bergman <jocke@gipset.se>

 databases/py-carbon/Makefile           | 4 ++--
 devel/py-buildbot-www/Makefile         | 3 ++-
 devel/py-twisted/Makefile              | 3 ++-
 mail/py-alot/Makefile                  | 3 ++-
 multimedia/syncplay/Makefile           | 4 ++--
 net-im/py-matrix-synapse/Makefile      | 3 ++-
 net-im/py-unmessage/Makefile           | 4 ++--
 net-p2p/deluge-cli/Makefile            | 3 ++-
 net/py-matrix-synapse-ldap3/Makefile   | 3 ++-
 security/cowrie/Makefile               | 3 ++-
 security/py-trustme/Makefile           | 2 +-
 sysutils/datadog-integrations/Makefile | 3 ++-
 www/py-autobahn/Makefile               | 3 ++-
 www/py-scrapy/Makefile                 | 3 ++-
 www/py-treq/Makefile                   | 3 ++-
 15 files changed, 29 insertions(+), 18 deletions(-)
Comment 19 commit-hook freebsd_committer freebsd_triage 2023-11-27 06:02:34 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=0e91c0d1538b6edba16e4d9b6d1828c61b634a14

commit 0e91c0d1538b6edba16e4d9b6d1828c61b634a14
Author:     Po-Chuan Hsieh <sunpoet@FreeBSD.org>
AuthorDate: 2023-11-27 06:01:52 +0000
Commit:     Po-Chuan Hsieh <sunpoet@FreeBSD.org>
CommitDate: 2023-11-27 06:01:52 +0000

    security/py-service_identity: Mark DEPRECATED and set EXPIRATION_DATE to 2024-01-31

    - While I'm here, fix path (devel/ -> security/)

    PR:             275167
    Reported by:    John Hein <jcfyecrayz@liamekaens.com>
    Approved by:    Axel Rau <axel.rau@chaos1.de> (maintainer)

 security/py-service_identity/Makefile | 4 ++++
 1 file changed, 4 insertions(+)
Comment 20 Bryan Drewery freebsd_committer freebsd_triage 2023-11-28 00:13:58 UTC
The change to py-buildbot-www requires this change in py-buildbot:

diff --git devel/py-buildbot/Makefile devel/py-buildbot/Makefile
index 3425bca28c..206e06f481 100644
--- devel/py-buildbot/Makefile
+++ devel/py-buildbot/Makefile
@@ -14,7 +14,7 @@ LICENSE_FILE= ${WRKSRC}/COPYING

 RUN_DEPENDS=   ${PYTHON_PKGNAMEPREFIX}alembic>=1.6.0:databases/py-alembic@${PY_FLAVOR} \
                ${PYTHON_PKGNAMEPREFIX}autobahn>=0.16.0:www/py-autobahn@${PY_FLAVOR} \
-               ${PYTHON_PKGNAMEPREFIX}buildbot-www==${PORTVERSION}:devel/py-buildbot-www@${PY_FLAVOR} \
+               ${PYTHON_PKGNAMEPREFIX}buildbot-www==${PKGVERSION}:devel/py-buildbot-www@${PY_FLAVOR} \
                ${PYTHON_PKGNAMEPREFIX}dateutil>=1.5:devel/py-dateutil@${PY_FLAVOR} \
                ${PYTHON_PKGNAMEPREFIX}Jinja2>=2.1:devel/py-Jinja2@${PY_FLAVOR} \
                ${PYTHON_PKGNAMEPREFIX}pyjwt>=0:www/py-pyjwt@${PY_FLAVOR} \
Comment 21 commit-hook freebsd_committer freebsd_triage 2024-01-31 22:24:56 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=243c56de93142efd8635beef1f61d14bbf622ba0

commit 243c56de93142efd8635beef1f61d14bbf622ba0
Author:     Rene Ladan <rene@FreeBSD.org>
AuthorDate: 2024-01-31 22:23:19 +0000
Commit:     Rene Ladan <rene@FreeBSD.org>
CommitDate: 2024-01-31 22:23:47 +0000

    security/py-service_identity: Remove expired port

    2024-01-31 security/py-service_identity: Use security/py-service-identity

    PR:             275167

 MOVED                                         |  1 +
 security/Makefile                             |  1 -
 security/py-service_identity/Makefile (gone)  | 27 ---------------------------
 security/py-service_identity/distinfo (gone)  |  3 ---
 security/py-service_identity/pkg-descr (gone) |  7 -------
 5 files changed, 1 insertion(+), 38 deletions(-)
Comment 22 John Hein 2024-02-07 14:12:10 UTC
I think this can be closed now.  The old port was deprecated and is now gone.  Dependent ports have been updated to point to the new port.


Note that comment 20 has not been addressed - separate bug.


(In reply to Palle Girgensohn from comment #17)
First let me say that I understand the position to force ports to use a newer version in some cases and started thinking along the same lines in this case at first.

I came down on the side of deprecating the older version mainly for the following reasons:

 (1) I don't know the ramifications of moving to a new update for all the affected ports.  I took a look, but I realized I don't know enough.  For example, for some of the ports that specify service-identity as a dependency, I don't see a direct dependency.  Maybe the service-identity dependency should just be removed for some of these ports.  If so, that's more correct - even better than globally updating their dependency to a newer version.

 (2) There is ('was' now) indeed a run-time conflict between some of these ports.  But, to be honest, most of these ports are not critical ports for the global ports tree.  I'm sure they are important to some, but I'm saying they are not globally critical.  To me, this indicates that we don't have to act without having a full understanding of the ramifications.  py-twisted is pretty important, and after some analysis, it seems that it can be updated to service-identity 23.1.0. But I don't have the time to analyze all the affected ports - at least not to a confidence level where I am comfortable with forcing them all to a new dependency version.

 (3) The maintainers of the affected ports should be able to evaluate and have time to weigh in on updates for their port.

Doing global updates to ports without maintainer feedback should be done sparingly unless the changes are obviously correct and necessary for the greater good.

I fully understand why one would want to do the sweeping patch.  But this didn't seem to rise to that level of emergency.  Allowing maintainers some time to review seems reasonable.  Build failures are much less problematic than run-time failures - the latter is harder to debug.

At the very least the maintainers of the affected ports should be explicitly invited to review and given time to evaluate.

Generally, regardless whether we go the "deprecate" route or the "update all affected ports now" route, we should invite all the affected maintainers to evaluate how changes will affect their port (including whether the port really does need a direct dependency on service-identity).
Comment 23 Palle Girgensohn freebsd_committer freebsd_triage 2024-02-07 14:54:37 UTC
(In reply to John Hein from comment #22)
Hi,

These are reasonable objections. Cool, I agree. We should keep all maintainers in the loop. In this case, it was a mistake to start with, to actually get two versions in to the ports tree. Seems it is fixed now.

Palle
Comment 24 Palle Girgensohn freebsd_committer freebsd_triage 2024-02-07 14:56:08 UTC
(In reply to Bryan Drewery from comment #20)
It is suggested that this is still required? In what way does it fail without this patch?