Created attachment 255432 [details] Patch to update SyncTERM to 1.2 Updates to v1.2 and silences some portlint warnings.
Created attachment 255435 [details] As above but with correct distinfo Sorry, I had the wrong distinfo in the original patch.
Created attachment 255436 [details] 1.3 is actually the newest version Version 1.2 was released with a critical bug, 1.3 is the current release.
1. Build error on live system 14.1 amd64: utf8.c:262:16: error: use of undeclared identifier 'cp437_unicode_tbl' 262 | codepoint = cp437_unicode_tbl[*p]; 2. Multiple build warnings in poudriere 14.1 amd64: /bin/sh: pkg-config: not found Need USES=pkgconfig 3. Stage errors in poudriere: ===> Checking for items in STAGEDIR missing from pkg-plist Error: Orphaned: share/icons/hicolor/16x16/apps/syncterm.png Error: Orphaned: share/icons/hicolor/22x22/apps/syncterm.png Error: Orphaned: share/icons/hicolor/24x24/apps/syncterm.png Error: Orphaned: share/icons/hicolor/256x256/apps/syncterm.png Error: Orphaned: share/icons/hicolor/32x32/apps/syncterm.png Error: Orphaned: share/icons/hicolor/36x36/apps/syncterm.png Error: Orphaned: share/icons/hicolor/48x48/apps/syncterm.png Error: Orphaned: share/icons/hicolor/scalable/apps/syncterm-mini.svg Error: Orphaned: share/icons/hicolor/scalable/apps/syncterm.svg Need update pkg-plist. 4. syncterm.desktop doesn't have MimeType - not need USES=desktop-file-utils. 5. Multiple warnings from portclippy. 6. You are a port committer, port without maintainer - want to become the maintainer?
3. pkg-plist bin/syncterm share/applications/syncterm.desktop share/icons/hicolor/16x16/apps/syncterm.png share/icons/hicolor/22x22/apps/syncterm.png share/icons/hicolor/24x24/apps/syncterm.png share/icons/hicolor/256x256/apps/syncterm.png share/icons/hicolor/32x32/apps/syncterm.png share/icons/hicolor/36x36/apps/syncterm.png share/icons/hicolor/48x48/apps/syncterm.png share/icons/hicolor/64x64/apps/syncterm.png share/icons/hicolor/scalable/apps/syncterm-mini.svg share/icons/hicolor/scalable/apps/syncterm.svg share/man/man1/syncterm.1.gz
7. SDL: Warning: you might not need LIB_DEPENDS on libSDL.so =========================================================================== =>> Checking shared library dependencies 0x0000000000000001 NEEDED Shared library: [libc.so.7] 0x0000000000000001 NEEDED Shared library: [libm.so.5] 0x0000000000000001 NEEDED Shared library: [libncursesw.so.9] 0x0000000000000001 NEEDED Shared library: [libthr.so.3] 0x0000000000000001 NEEDED Shared library: [libtinfow.so.9] 0x0000000000000001 NEEDED Shared library: [libutil.so.9] =======================<phase: deinstall >============================ Doesn't use SDL? Or uses it via dlopen?
1. devel/libunicode install ${LOCALBASE}/include/unicode.h and comms/syncterm grab it silently instead of embedded src/xpdev/unicode.h. Trying to force use embedded…
1. Workaround: CFLAGS+= -I../../src/xpdev
Thanks for all the feedback, I'll update the patch later today. 1) I'll add that, thanks. 2) pkgconfig shouldn't be required unless things are installed somewhere weird. 3) I'll update the plist well. 4) Roger, will remove it. 5) Will take a look. 6) I didn't realize I still had my ports bit, I thought it had been taken in for safekeeping. 7) Both SDL and X11 are used via dlopen and are technically only build dependencies. The same is true for various audio libraries which don't currently have OPTIONS. I can add those options as well and make the libraries build depends.
6. I don't know - just checked commits: https://cgit.freebsd.org/ports/log/?qt=committer&q=shur 7. BUILD_DEPENDS + RUN_DEPENDS instead of LIB_DEPENDS. But for dependencies via USES I would leave it as is. Better fix deps from other libs - SDL sound?
Created attachment 255450 [details] Updated with feedback Initial incorporation of feedback, not tested yet as I'm now updating all the ports on my desktop development system.
I think this is incorrect: MAKE_ARGS+= WITHOUT_PORTAUDIO=yes PORTAUDIO_MAKE_ARGS_OFF= NO_PULSEAUDIO=1
Also I think better add pkgconfig to global USES.
Comment on attachment 255450 [details] Updated with feedback IMHO, this look better: MAKE_ARGS= -C ${WRKSRC} \ PREFIX="${PREFIX}" \ MANPREFIX="${PREFIX}/share" \ INSTALL_EXE="${INSTALL_PROGRAM}" \ RELEASE=1 than this: MAKE_ARGS+= -C ${WRKSRC} MAKE_ARGS+= PREFIX="${PREFIX}" MAKE_ARGS+= MANPREFIX="${PREFIX}/share" MAKE_ARGS+= INSTALL_EXE="${INSTALL_PROGRAM}" MAKE_ARGS+= RELEASE=1 If PLIST_FILES longer than 6 lines => move to file pkg-plist.
Incorrect syntax: ALSA_BUILD_DEPENDS= audio/alsa-lib PORTAUDIO_BUILD_DEPENDS= audio/portaudio PULSEAUDIO_BUILD_DEPENDS= audio/pulseaudio And IMHO better LIB_DEPENDS instead BUILD_DEPENDS: ALSA_LIB_DEPENDS= libasound.so:audio/alsa-lib PORTAUDIO_LIB_DEPENDS= libportaudio.so:audio/portaudio PULSEAUDIO_LIB_DEPENDS= libpulse.so:audio/pulseaudio
(In reply to Vladimir Druzenko from comment #14) Or: ALSA_BUILD_DEPENDS= alsa-lib>0:audio/alsa-lib ALSA_RUN_DEPENDS= alsa-lib>0:audio/alsa-lib PORTAUDIO_BUILD_DEPENDS= portaudio>0:audio/portaudio PORTAUDIO_RUN_DEPENDS= portaudio>0:audio/portaudio PULSEAUDIO_BUILD_DEPENDS= pulseaudio>0:audio/pulseaudio PULSEAUDIO_RUN_DEPENDS= pulseaudio>0:audio/pulseaudio
I've been very slack on keeping my desktop system up to date, so it looks like it'll be a couple days rebuilding ports before I can get back to this. The main reason for BUILD_DEPENDS only is because SyncTERM uses dlopen() for optional libraries, and with binary packages being the norm now, being able to install the binary package without needing to pull in X11, SDL, etc. is very nice. That said, doing the update without changing how the dependencies work first, then changing it later seems completely reasonable.
(In reply to Stephen Hurd from comment #8) > pkgconfig shouldn't be required unless things are installed somewhere weird. As we support building with custom PREFIX and LOCALBASE values, please use pkgconfig instead of hoping that it works without. If the dependencies are indeed optional, I don't see why they need to be registered as dependencies. Instead, maybe it is preferable to add an appropriate note to pkg-message so the user knows what to install should extra features be needed.
(In reply to Robert Clausecker from comment #17) Yeah, the current patch adds pkg-config for the features it is used for, but as Vladimir points out, it's likely overkill to be careful to only USES pkgconfig when we know it will be used. As for a pkg-message note about which optional libraries can change functionality, that would mean re-writing pkg-message based on the OPTIONS... I'm not sure if you can actually do that.
(In reply to Stephen Hurd from comment #18) I was more thinking of something along the lines of not adding the optional dependencies as RUN_DEPENDS and just having pkg-message say “Syncterm has been installed. To access features x, y, and z, install packages a, b, c.” to leave it up to the user to install the packages if desired.
(In reply to Robert Clausecker from comment #19) The issue with that is that some people (such as myself) don't want to use portaudio or pulseaudio in any software where it's not actually required, but do have them installed. Other people may want to use pulseaudio for everything if possible. There's really no one-size-fits-all for this one.
Created attachment 255555 [details] Simplified update This one doesn't have the various sub-X11 options. the only reason I can think of to disable those is to work around bugs or if the platform doesn't support them. The ALSA option is also removed since it turns out you can't actually build on FreeBSD with ALSA support. Default audio is just OSS, default graphical output is just X11, and the SDL option controls both SDL video and SDL audio. This should cover all real use cases that aren't bug workarounds, and defaults to directly using the FreeBSD sound system.
Thanks. This looks great. On commit, I'll use DISTVERSION over PORTVERSION as per policy. The port also builds fine on aarch64, so I'll add that to the list of supported architectures. Why do you believe it should not build on architectures other than those you have listed? On armv7 it fails as follows: cc -c -D__UNIX__ -DNDEBUG -I. -DDATA_LITTLEENDIAN -DFIXED_SEED=0xdb933068 -DHAS_DEVCRYPTO -fstack-protector-strong -D_FORTIFY_SOURCE=2 -Wno-pointer-sign -Wno-switch -fwrapv -fno-delete-null-pointer-checks -fPIC -DHAS_RECURSIVE_MUTEX -DHAS_ROBUST_MUTEX -DOSVERSION=14 -fomit-frame-pointer -pthread -o ./static-obj/int_err.o misc/int_err.c misc/int_err.c:44:14: error: invalid operands to binary expression ('va_list' (aka '__builtin_va_list') and 'void *') 44 | REQUIRES_B( verifyVAList( argPtr ) ); | ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~ ./misc/os_spec.h:628:37: note: expanded from macro 'verifyVAList' 628 | #define verifyVAList( x ) ( ( x ) != NULL ) | ^ ~~~~ ./misc/safety.h:26:33: note: expanded from macro 'REQUIRES_B' 26 | #define REQUIRES_B( x ) if( !( x ) ) retIntError_Boolean() | ^ This seems like a wrong assertion; va_list is not guaranteed to be a pointer type. Could I interest you in maintainership over this port?
I also suggest replacing: MAKE_ARGS+= -C ${WRKSRC} MAKE_ARGS+= PREFIX="${PREFIX}" MAKE_ARGS+= MANPREFIX="${PREFIX}/share" MAKE_ARGS+= INSTALL_EXE="${INSTALL_PROGRAM}" MAKE_ARGS+= RELEASE=1 with MAKE_ARGS= -C ${WRKSRC} \ PREFIX="${PREFIX}" \ MANPREFIX="${PREFIX}/share" \ INSTALL_EXE="${INSTALL_PROGRAM}" \ RELEASE=1
(In reply to Robert Clausecker from comment #22) I have no objections to being made the maintainer.
Created attachment 255569 [details] Accept maintainership, fix armv7 build(?) Updated with feedback and taking maintainership. This should fix the armv7 failure that was reported... no guarantee it's not broken elsewhere.
Created attachment 255570 [details] Accept maintainership, fix(?) armv7 build Re-uploading because I forgot to mark the old one as obsolete (whoops!)
Created attachment 255571 [details] Whoops, forgot to commit the MAINTAINER change Updated with me as maintainer.
Created attachment 255572 [details] Argh, and obsolete the old patches. That's it, done for the night.
Created attachment 255573 [details] Fix patch name to silence portlint
(In reply to Stephen Hurd from comment #29) Note that you can click on "details" of an existing attachment and then mark it as obsolete there. No need to upload a new one.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=cc310247020d50af5b60c7893cce0fd7d3c300e5 commit cc310247020d50af5b60c7893cce0fd7d3c300e5 Author: Stephen Hurd <shurd@FreeBSD.org> AuthorDate: 2024-11-24 21:46:58 +0000 Commit: Robert Clausecker <fuz@FreeBSD.org> CommitDate: 2024-12-02 14:27:35 +0000 comms/syncterm: Update to 1.3 Updates SyncTERM to 1.3 and fixes some portlint errors. Submitter becomes maintainer. PR: 282954 comms/syncterm/Makefile | 63 +++++++++++----------- comms/syncterm/distinfo | 5 +- .../files/cl-dont-validate-va-list.patch.in (new) | 11 ++++ .../files/patch-3rdp_build_GNUmakefile (new) | 10 ++++ comms/syncterm/pkg-plist (new) | 13 +++++ 5 files changed, 70 insertions(+), 32 deletions(-)
Thank you for your contribution. I hope you'll get your commit bit back soon.