Skip to content

bump miniupnpc to 2.2.8 #6907

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 14, 2024
Merged

bump miniupnpc to 2.2.8 #6907

merged 6 commits into from
Jun 14, 2024

Conversation

Coeur
Copy link
Collaborator

@Coeur Coeur commented Jun 10, 2024

bump miniupnpc from 2.2.7 to to 2.2.8

@Coeur Coeur marked this pull request as draft June 10, 2024 15:26
@Coeur
Copy link
Collaborator Author

Coeur commented Jun 10, 2024

When miniupnpc is 2.2.7 or older, UPNP_GetValidIGD requires exactly 5 arguments.
When miniupnpc is 2.2.8 or newer, UPNP_GetValidIGD requires exactly 7 arguments.
This is exactly the type of situations where they should have upgraded the Major version, or better, adopted a different method name. :/

[edit]
3523b92

@Coeur Coeur marked this pull request as ready for review June 10, 2024 15:34
@Coeur Coeur added this to the 4.1.0 milestone Jun 10, 2024
@mikedld
Copy link
Member

mikedld commented Jun 10, 2024

You seem to be referencing a commit that's not in our mirror (https://github.com/transmission/miniupnp), at least I'm not seeing a tag named miniupnpc_2_2_8 there, it'll need to be synced (including tags, I'd say) first. Could be that it works because GitHub transparently looks the commit up in the upstream repo, same way it works vice versa (referring to a commit in a fork repo through the upstream repo), but it doesn't mean it's the right thing to do because it's GitHub's magic / implementation details.

If we're currently referencing 2.2.7 (not sure when that happened), that's a good thing I noticed because if upstream would go away right now we'd have a broken repo.

@mikedld
Copy link
Member

mikedld commented Jun 10, 2024

Ah, looks like we do have a commit matching 2.2.7 there after all, just not the tag :-\

@Coeur
Copy link
Collaborator Author

Coeur commented Jun 11, 2024

That has been discussed in the past: I offered Charles to have write access to the 3rd-party forks to sync them, but he preferred to do it himself whenever there are 3rd-party PR changes.

So I'm not doing it wrong: that's the current process:

  1. We make a PR here on Transmission updating a 3rd-party
  2. One of the 3 admins of Transmission syncs the forks
  3. One of the 3 admins of Transmission merges the PR

And, well, in the extreme case where a 3rd-party would be erased from existence right after a release, I do have a local copy, since I test my changes before making pull requests, but I would question the validity of such release if the sources are no more official.

@tearfur tearfur added the dependencies Pull requests that update a dependency file label Jun 11, 2024
@Coeur Coeur requested a review from tearfur June 11, 2024 13:32
Copy link
Member

@mikedld mikedld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've synced all our 3rd-party mirrors (and set it up to sync periodically), so that should be resolved.

Coeur and others added 2 commits June 12, 2024 13:12
Co-authored-by: Mike Gelfand <mikedld@users.noreply.github.com>
Co-authored-by: Mike Gelfand <mikedld@users.noreply.github.com>
@Coeur Coeur requested a review from mikedld June 12, 2024 05:19
Copy link
Member

@tearfur tearfur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't comment on the Xcode changes, but I'm happy with the other changes.

See if Mike wants the #if block extracted to a helper function instead.

@Coeur
Copy link
Collaborator Author

Coeur commented Jun 12, 2024

See if Mike wants the #if block extracted to a helper function instead.

I prefer not: for a single usage, I prefer avoiding wrappers.

Can't comment on the Xcode changes, but I'm happy with the other changes.

man ln for the doc about that function, ah ah. Basically, the option n is to help overwrite the symbolic link when changing the version of miniupnp.

@mikedld mikedld merged commit febfe49 into transmission:main Jun 14, 2024
25 checks passed
@Coeur Coeur deleted the coeur/miniupnp branch June 16, 2024 05:41
fornwall added a commit to termux/termux-packages that referenced this pull request Jul 6, 2024
It seems that miniupnpc 2.2.8 broke API compatibility - see
miniupnp/miniupnp#758.

This causes the current transmission package to segfault (#20490) as
well as the build to fail, so we here backport
transmission/transmission#6907 to fix that.

Closes #20490
fornwall added a commit to termux/termux-packages that referenced this pull request Jul 6, 2024
It seems that miniupnpc 2.2.8 broke API compatibility - see
miniupnp/miniupnp#758.

This causes the current transmission package to segfault
as well as the build to fail, so we here backport
transmission/transmission#6907 to fix that.

Corresponding `transmission` patch: #20788
fornwall added a commit to termux/termux-packages that referenced this pull request Jul 7, 2024
It seems that miniupnpc 2.2.8 broke API compatibility - see
miniupnp/miniupnp#758.

This causes the current transmission package to segfault (#20490) as
well as the build to fail, so we here backport
transmission/transmission#6907 to fix that.

Closes #20490
fornwall added a commit to termux/termux-packages that referenced this pull request Jul 7, 2024
It seems that miniupnpc 2.2.8 broke API compatibility - see
miniupnp/miniupnp#758.

This causes the current transmission package to segfault
as well as the build to fail, so we here backport
transmission/transmission#6907 to fix that.

Corresponding `transmission` patch: #20788
termux-pacman-bot added a commit to termux-pacman/termux-packages that referenced this pull request Jul 7, 2024
It seems that miniupnpc 2.2.8 broke API compatibility - see
miniupnp/miniupnp#758.

This causes the current transmission package to segfault (#20490) as
well as the build to fail, so we here backport
transmission/transmission#6907 to fix that.

Closes #20490
termux-pacman-bot added a commit to termux-pacman/termux-packages that referenced this pull request Jul 7, 2024
It seems that miniupnpc 2.2.8 broke API compatibility - see
miniupnp/miniupnp#758.

This causes the current transmission package to segfault
as well as the build to fail, so we here backport
transmission/transmission#6907 to fix that.

Corresponding `transmission` patch: #20788
ksv1986 added a commit to ksv1986/transmission that referenced this pull request Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file scope:3rdparty
Development

Successfully merging this pull request may close these issues.

3 participants