Comment 1 Zsolt Udvari 2024-03-31 18:34:54 UTC

Could you please update to 2.51?
Could you please check "portfmt -D Makefile" and "portclippy Makefile" (ports-mgmt/portfmt)?
Thanks!

I've been traveling.  I will update today.

Created attachment 249679 [details]
Upgrade to pgbackrest 2.51

Update to pgbacrest 2.51, fix portfmt/portclippy issues.

Comment 4 Zsolt Udvari 2024-04-06 05:32:05 UTC

Thanks.
What do you think the "note to packagers"? https://github.com/pgbackrest/pgbackrest/releases/tag/release%2F2.51
"NOTE TO PACKAGERS: The build system for pgBackRest is now meson. The autoconf/make build will not receive any new features and will be removed after a few releases."

Do you change to meson at the next version?

Well, thats cheerful news. I will take a stab at changing to meson.

Comment 6 Zsolt Udvari 2024-04-06 20:20:58 UTC

(In reply to Michael Schout from comment #5)
Okay, I'll wait it. Thanks.

Created attachment 249837 [details]
Ugrade to pgbackrest 2.51, switch to meson, add flags for liblz4 and libssh2

I switched the build system to meson and took the time to pull in config args for new features.

- Switch to meson build system
- Added patch for src/meson.build to make "meson install" actually install the binary
- Added config options for optional libs: liblz4, libssh2

Didn't see any obvious way to replicat the "autoconf fatal if optional lib is missing" in meson, so I dropped these:

ZSTD_CONFIGURE_OFF=	ac_cv_lib_zstd_ZSTD_isError=no
ZSTD_CONFIGURE_ON=	ac_cv_lib_zstd_ZSTD_isError=yes


I suspect it would involve significant gymnastics if we were to try to replicate that in meson.  If I'm mistaken let me know.

Comment 8 Zsolt Udvari 2024-04-09 09:06:36 UTC

(In reply to Michael Schout from comment #7)
Thanks!

You should use USES=python instead of BUILD_DEPENDS (see https://docs.freebsd.org/en/books/porters-handbook/book/#using-python and https://cgit.freebsd.org/ports/tree/Mk/Uses/python.mk#n7). The poudriere reports it ("Error: lang/python3 should not be depended upon. Instead, use USES=python:xy with a specific version.").

Options: don't you set any meson-variable? If a user build this port with "make install" and the liblz4 is installed on her machine the pgbackrest will use it but the LZ4 option is off.
What do you think: are needed these options?
The lz4, ssh2, zstd is widely used? If yes remove them from options and simply add to LIB_DEPENDS.

I'll fix USES python today.

As for the options:

- ssh2 is almost certainly *not* widely used as it was not even available until the past couple of releases upstream.
- zstd, same situation, but added longer ago.
- lz4 has been there for very long time and we should probably just depend on that one automatically.


And yeah, if they do "make install" with e.g. lz4 turned off but its installed, the port is going to use it.  Will see if I can work out how to get around that (IE explicitly turn it off).

Ok, worked out how get the features disabled.  Will involve some patching of `meson.build`.  When done will submit new patch. Will also send the build patches upstream to make this easier in future releases.

Created attachment 249860 [details]
Explicitly disable deps that are turned off, switch to meson builds, update to v2.51

Updated patch which ensures we do not link against any options that are disabled.

I think this addresses all remaining concerns.  Please let me know if there is anything else thanks!

Created attachment 249876 [details]
Use meson build system, update to 2.51, add SFTP config option

I updated the patch to use the one that got merged upstream.

I also updated the SSH option description to match the one from upstream (it is only used to enable SFTP support).

I am good to get this merged now.

Created attachment 249877 [details]
Use meson build system, update to 2.51, add SFTP config option

Fix portclippy warnig.

Created attachment 249878 [details]
Use meson build system, update to 2.51, add SFTP config option

Accidentally updloaded wrong patch file last time, sorry.

Comment 15 Zsolt Udvari 2024-04-13 13:15:13 UTC

Thanks.

The post-patch target: if you want use sed, it's better the REINPLACE_CMD (see https://docs.freebsd.org/en/books/porters-handbook/book/#slow-patch-automatic-replacements).
In your special case the best solution is shebangfix: https://docs.freebsd.org/en/books/porters-handbook/book/#uses-shebangfix

Comment 16 Zsolt Udvari 2024-04-13 13:20:22 UTC

(In reply to Michael Schout from comment #12)
"I also updated the SSH option description to match the one from upstream (it is only used to enable SFTP support)."
If this option enables the SFTP support, maybe want rename the option SSH to SFTP? I think it would be more logic and the SSH option is a base option (https://cgit.freebsd.org/ports/tree/Mk/bsd.options.desc.mk#n478).

Comment 17 Zsolt Udvari 2024-04-13 17:49:36 UTC

(In reply to Zsolt Udvari from comment #16)
"shebangfix": sorry, I misunderstood your patch. In your case should use BINARY_ALIAS (https://docs.freebsd.org/en/books/porters-handbook/book/#binary-alias).

Created attachment 249971 [details]
Update to pgbackrest 2.51, use meson build system, add SFTP config option

Latest patch which I think addresses all of your concerns.

If anything else is needed please let me know

Comment 19 Zsolt Udvari 2024-04-14 16:16:42 UTC

(In reply to Michael Schout from comment #18)
Thanks for your hard work. IMHO this patch is perfect but should review by mentors: https://reviews.freebsd.org/D44791

Comment 20 Zsolt Udvari 2024-04-21 10:45:26 UTC

(In reply to Zsolt Udvari from comment #19)
Could you please check https://reviews.freebsd.org/D44791#inline-270417 and https://reviews.freebsd.org/D44791#inline-270416 ?

Comment 21 Zsolt Udvari 2024-04-21 10:47:45 UTC

https://reviews.freebsd.org/D44791#1023359

Regarding the review comments about where the patches originate, they are direct from upstream, added after 2.51 was released.

When 2.52 is available these patches wont be part of the port any longer.  They are only here for purposes of updating FreeBSD to v2.51.

See: https://github.com/pgbackrest/pgbackrest/commit/7b95fd3bd29346e7325aa161bfee45efd71e22eb

I'll change the makefile to use helpers, but wont have time until tomorrow at earliest.

I can see with upstream about changing how to find the pythong binary.  Not sure if that one would be worth holding up the 2.51 update for or not.

Created attachment 250182 [details]
Use meson build system, update to 2.51, add SFTP config option

Addressed MR comments:

- Use Helpers for MESON config options

I submitted the "python3" path patch upstream.  If its accepted, we can just remove the "BINARY_ALIAS" at next release.

Please let me know if there is anything else as this port is outdated now for quite some time.

The patch to dynamically find "python" got merged upstream.  Again, will be in 2.52 but is not in current release version.

I'd prefer to stick with the short couple-lines patch I made here meanwhile since the upstream one adds a lot more changes for CI and tests.

In 2.52 we should not need any local patches as all of the issues were addressed upstream.

See: https://github.com/pgbackrest/pgbackrest/pull/2338

Regarding your comment in review:

> Use of GH_TAGNAME would be better than patches? Or the Example 18. in Porter's Handbook?

This is a bad idea IMO.

We should *NOT* base the package on an unreleased upstream version that potentially has other non-build-related unproven changes.

Especially for something as mission critical as "pgbackrest"

We should stick to released code only.  The patches involved here are solely for the build and nothing else.

I would further propse that if pulling in the upstreamed build patches here is somehowi problematic we should just skip 2.51 and wait for the next release.

Alternatively, drop use of meson for 2.51 and revisit in 2.52 when patches aren't needed.

Comment 30 commit-hook 2024-04-27 05:49:14 UTC

A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=43ecb93463973dbe210d4fd5dc2158b962a012bc

commit 43ecb93463973dbe210d4fd5dc2158b962a012bc
Author:     Michael Schout <schoutm@gmail.com>
AuthorDate: 2024-04-14 16:08:30 +0000
Commit:     Zsolt Udvari <uzsolt@FreeBSD.org>
CommitDate: 2024-04-27 05:48:06 +0000

    databases/pgbackrest: update to 2.51

    Switch to meson build system.
    Rename option SSH to SFTP.

    The origin of patch is
    https://github.com/pgbackrest/pgbackrest/commit/7b95fd3bd29346e7325aa161bfee45efd71e22eb
    It is upstreamed and can remove it at update to 2.52.

    Changelog: https://pgbackrest.org/release.html#2.51

    PR:             276559
    Approved by:    diizzy (mentor)
    Differential Revision:  https://reviews.freebsd.org/D44791

 databases/pgbackrest/Makefile                      | 30 +++++++++++++---------
 databases/pgbackrest/distinfo                      |  6 ++---
 databases/pgbackrest/files/patch-meson.build (new) | 28 ++++++++++++++++++++
 .../files/patch-meson__options.txt (new)           |  8 ++++++
 .../pgbackrest/files/patch-src_meson.build (new)   | 10 ++++++++
 5 files changed, 67 insertions(+), 15 deletions(-)

Comment 31 Zsolt Udvari 2024-04-27 05:50:25 UTC

Committed, thanks!