Skip to content

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Feb 1, 2023

Version 17 is currently the latest version, see: https://github.com/miniupnp/miniupnp/blob/master/miniupnpc/apiversions.txt, and has been available since the release of 2.1. 2.1 or newer is readily available across all distros, see https://repology.org/project/miniupnpc/versions, so drop support for the older API versions.

Split out of #22644.

Version 17 is currently the latest version, and has been available since
the release of 2.1.
See: https://github.com/miniupnp/miniupnp/blob/master/miniupnpc/apiversions.txt.
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 1, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK hebasto, TheCharlatan
Concept ACK dergoegge

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #22644 (Deprecate UPnP support, require 2.1 or later by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@dergoegge
Copy link
Member

Concept ACK

@hebasto
Copy link
Member

hebasto commented Feb 3, 2023

Concept ACK.

@hebasto
Copy link
Member

hebasto commented Feb 3, 2023

Now we can use PKG_CHECK_MODULES instead of

bitcoin/configure.ac

Lines 1414 to 1418 in 7753efc

AC_CHECK_HEADERS(
[miniupnpc/miniupnpc.h miniupnpc/upnpcommands.h miniupnpc/upnperrors.h],
[AC_CHECK_LIB([miniupnpc], [upnpDiscover], [MINIUPNPC_LIBS="$MINIUPNPC_LIBS -lminiupnpc"], [have_miniupnpc=no], [$MINIUPNPC_LIBS])],
[have_miniupnpc=no]
)
as pkgconfig/miniupnpc.pc is being provided with a newer libminiupnpc-dev package across of all major distros and macOS's Homebrew.

@fanquake
Copy link
Member Author

fanquake commented Feb 3, 2023

Now we can use PKG_CHECK_MODULES instead of

That doesn't quite work. But is already part of #22644 in any case.

@hebasto
Copy link
Member

hebasto commented Feb 3, 2023

Now we can use PKG_CHECK_MODULES instead of

That doesn't quite work.

I'm not sure what it means, but my point is that using PKG_CHECK_MODULES makes the MINIUPNPC_API_VERSION check redundant.

@fanquake
Copy link
Member Author

fanquake commented Feb 3, 2023

I'm not sure what it means,

You can't use it, because it's not supported for mingw-w64 builds using 2.1. So while we could patch depends, distro/self-builds would be broken.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK b3b673f, tested on Ubuntu 20.04 w/ and w/o libminiupnpc-dev package.

Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

ACK b3b673f

Tested against miniupnpc version 2.0 (API version 16) and it failed as expected. Also tested against system libminiupnpc-dev on Ubuntu 22.04.

@fanquake fanquake merged commit 1ad0711 into bitcoin:master Feb 13, 2023
@fanquake fanquake deleted the mostly_non_broken_upnp_verions branch February 13, 2023 16:26
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 14, 2023
b3b673f mapport: require miniupnpc API version 17 or later (fanquake)

Pull request description:

  Version 17 is currently the latest version, see: https://github.com/miniupnp/miniupnp/blob/master/miniupnpc/apiversions.txt, and has been available since the release of 2.1. 2.1 or newer is readily available across all distros, see https://repology.org/project/miniupnpc/versions, so drop support for the older API versions.

  Split out of bitcoin#22644.

ACKs for top commit:
  hebasto:
    ACK b3b673f, tested on Ubuntu 20.04 w/ and w/o [`libminiupnpc-dev`](https://packages.ubuntu.com/focal/libminiupnpc-dev) package.
  TheCharlatan:
    ACK b3b673f

Tree-SHA512: f53b36b82462c4ea83d9b83413dca8097885d1620f7ca0a53a79d6b3d3cf37c7773828b23f4278ccfcc3b14fcb0faffa35f60191b519b04570f3d2783d0303e2
@bitcoin bitcoin locked and limited conversation to collaborators Feb 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants