Created attachment 244076 [details] lang/python311: backport netlink support this patch backports GH-107812: extend socket's netlink support to FreeBSD see https://github.com/python/cpython/pull/107813 for the patch. see https://github.com/python/cpython/issues/107812 for the discussion on the backport itself. Sponsored by: The FreeBSD Foundation related bug #273114
Currently doing some light testing on my end. Especially since the actual change was accepted upstream as one commit (not counting the NEWS entry), and that you did the work in case they would accept the backports, please use ${PATCH_SITES}/${PATCHFILES} here, like https://github.com/python/cpython/commit/eaa3bd080ef2299cbe614c348e3b8fdfacb66b48. Not only easier to keep track of, but ensures that this gets applied before anything in ${PATCHDIR}.
i would've done that, if it applied. The only branch it applies on is 3.12, and we don't currently have a port for that.
You need to redo the existing ${PATCHDIR} patches on top of your stuff accepted upstream.
it makes no sense to use https://github.com/python/cpython/commit/eaa3bd080ef2299cbe614c348e3b8fdfacb66b48 in ${PATCH_SITES}/${PATCHFILES} as GitHub says: > This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository. I don't wanna use a mystery commit on a mystery fork just because it's on GitHub. that fork is mine, but it could go away any time: if I forget that there's anything relevant in that fork, i could go on a cleaning spree and delete that fork. and what do we do then?
(In reply to Charlie Li from comment #3) Charlie, I'm not following what you are asking. Could you clarify? Comment #1 Mina's changes were only committed to upstream's main branch, but she wants to apply the same changes to these releases. Defining ${PATCH_SITES}/${PATCHFILES} isn't going to work, because the commit won't apply against any of python 3.8, 3.9, 3.10, or 3.11. Comment #3 I'm unclear why you are asking for the patches to be made on top of something accepted in upstream's main branch. Are we misunderstanding you?
(In reply to Mina Galić from comment #4) It's not a mystery if you comment the URL of the upstream issue in the ${PATCHFILES} line. The commit also lists the original issue. If you are that worried, create some backport issues upstream but immediately close them, that way the commits stay even after any branch deletion. (In reply to Joseph Mingrone from comment #5) They do work by using the "ghost" commits. (In reply to Joseph Mingrone from comment #5) ${PATCH_SITES}/${PATCHFILES} are applied before ${PATCHDIR} so the order matters. Since what was accepted upstream, then backported, touches files that ${PATCHDIR} does, ${PATCHDIR} patches need to be regenerated. And since this was accepted upstream, even just in trunk, they tacitly recommended that we carry it ourselves for the backports, so ${PATCH_SITES}/${PATCHFILES} they go.
(In reply to Charlie Li from comment #6) I get what you are asking for now, but are all these hoops to jump through necessary. Mina's proposed changes overlap by one line in one file (configure)? Can't she just combine the changes into one configure patch?
(In reply to Charlie Li from comment #6) Why should you abuse GitHub's garbage collection or rather non/slow garbage collection/ghost feature and also upstreams repo? If there's concern about keeping track why not include the backport in a single patch file, add a comment of origin within the patch and name it like patch-git-01-<upstream-full-hash> and commit? Done and everyone is happy?
We've done this with at least earlier iterations of graphics/mesa{,-devel}, and it all works as intended. It's not just about keeping track, but ensuring that everything is applied in the correct order, and that any local patches are based on the original source plus items they accepted whether in the same branch or trunk. Combining upstream patches with our local patches, sometimes resulting in the same local patch file (names) in ${PATCHDIR}, is just a bad idea. (In reply to Joseph Mingrone from comment #7) This is necessary when considering maintenance, semantics and mechanics. (In reply to Daniel Engberg from comment #8) Python upstream brought this upon themselves. In many other software projects, this would be a bug fix, not a new feature. If anyone there cares enough, they can read the last couple comments in the original issue, maybe see this exchange, and understand/remind themselves that asking distributions to carry arbitrary stuff they accepted as local patches comes at a cost. Don't forget that some other upstreams don't bother to backport anything, not even bug or build fixes, forcing downstreams like us to carry stuff. The single patch file doesn't address the ordering. ${PATCHDIR} really isn't meant for something like this.
(In reply to Charlie Li from comment #9) > Python upstream brought this upon themselves. In many other software projects, this would be a bug fix, not a new feature. If anyone there cares enough … if i create four pull requests that i know will be rejected, what does that look like? how likely is it then, after doing something like that, that my next legitimate patch will be accepted? upstream has explained their process. I don't agree with it, but shitting on it will not help future collaboration. If you don't want to merge these patches, that's fine. We'll close these bugs, and I'll wait for 3.13.
(In reply to Mina Galić from comment #10) Just close the pull requests yourself "immediately" after creation. No harm no foul, I've done it myself, albeit not as immediately and after triage (issue/pull request was obviated by other events a few weeks later). Distributions like us ultimately vote with our feet, and such is noticed, particularly during something like a PEP process. (cough cough Debian) This series of PRs wouldn't be set to "In Progress" if there was no immediate intent to commit them once issues not related to the code being patched are resolved.
We are free to integrate third-party packages, including Python, into our system, and integrate well, as we see fit -- license permitting. The entire discussion, both upstream and here, is taking WAY more time and space than the actual solution. The gist of the integration improvement is telling Python's configure script to look for netlink/netlink.h in addition to linux/netlink.h. If we find configure patches unwieldy, we can patch the source file and regenerate configure at build time, unless that would create circular dependencies in the tree. If we can't use patches for whatever reason, then we can use REINPLACE_CMD to keep the change really minimal, and it's possibly even good enough. Are there particular technical or quality concerns? Else if we deem it necessary to limit the patch to builds on FreeBSD 13.(>=2) and >= 14.0 because the older releases' netlink support were present but unsuitable, then please clearly mention so. So can we please stop the useless discussion of where and how to host this and cease bitching about Python's feature freeze policies, and just put either that patch and the REINPLACE_CMD, with the reference to the upstream 3.12 discussion, and add that stuff into our ports/ repository, test things, and then push (with bumped PORTREVISION). Feel free to convince the Python maintainers that this is a fix of the configure bug, but do it in parallel and don't wait for the result we may never have. Thank you.
People have had enough time to consider, I think we should move this along now. - mandree, I assume your previous comment is w/out portmgr hat - vishwin, is there any practical concern about applying these patches as is? While I'm not part of python@ team this seems like it's ready to land. I'm not sure of etiquette here but this ticket is currently un-assigned. If there are no objections, I could pick it up in 2 weeks after maintainer timeout, if its not dealt with prior.
(autotools) configure patches, against the generated-by-autoconf output, are unwieldy by default; this is no exception. The existing local as in ${PATCHDIR} patch against configure deals with LTO support, which is unlikely to be merged upstream. Nothing to do with netlink support, which has been merged upstream (even though trunk only). Mixing these two concerns in the same local patch(es) is a maintenance issue that is easily and should be avoided. Amongst other things, consider that, when a future ports person starts working on lang/python313 by repocopying lang/python312 (which will contain the netlink patch), they can immediately remove the ${PATCH_SITES}/${PATCHFILES} lines rather than figure out what went wrong in ${PATCHDIR} in this area. So no, these discussions are necessary, and expect more of them. If there is an unwillingness by the original submitter to do what is necessary, I will do it myself.
I am not a member of portmgr@. But I still feel, as a long-time FreeBSD committer, that there is a certain amount of obstructionism going on here that annoys me, in the form of requiring meena to jump through hoops that are brittle IMO. I cannot currently muster the resources to move this, so I will only repeat that this needs to move forward. Make the relevant patches for the FILES/PATCHDIR, mention how they come about as a header before the patchfile itself, and remember that ordering for files/patch-* is implicit by applying them in alphabetical order, so patch-aa and thereabouts would work if the natural order does not work. If - I have not looked at this patch nor other versions again, only at the discussion - patches overlap and one of them is trivial and can be applied later, make it a REINPLACE_CMD to orthogonalize it. But seriously this should happen now, and the patch then applied to all other Python branches that need it, too.
Created attachment 245298 [details] lang/python311: backport netlink support - implement patch as single file. - patch configure.ac instead of configure (as per Handbook) - add USES= autoreconf:2.69 - get rid of old patch-configure for LTO - implement it as patch-configure.ac, lest it be overwritten That last part could be done as REINPLACE_CMD, but for now I find it easier to have all patches … as patches. The LTO stuff should be fixed by making use of the previously detected cc_is_clang and pushed then upstream.
bumping this bug because it hasn't seen a reply in 3 months, because I just stumbled over bug 267648, which does basically the same thing, and now I'm wondering if I should've modelled my netlink patches after those patches? also: do we need an exp-run? etc…