Switch to pre-rolled release Use CMake to install the files instead of using custom do-install target. Unbundle devel/microsoft-gsl. Remove redundant USES=qt:5 instances. Only install cross-complied libraries on amd64. Pet portclippy (1).
Created attachment 244191 [details] audio/mumble: use pre-rolled release (apply with git am)
Build tests done: 12.4-amd64: OK 13.2-i386: OK CURRENT-amd64: OK Run tests done: CURRENT-amd64: OK
Check patch to this PR: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=272881
Can you send to upstream patches for overlay_gl/CMakeLists.txt and src/CMakeLists.txt?
(In reply to Vladimir Druzenko from comment #3) My patch should fix all of that. The overlay library should only be suffixed if built on amd64.
(In reply to Vladimir Druzenko from comment #4) Sure, I can upstream them.
(In reply to Jason E. Hale from comment #5) > Only install cross-complied libraries on amd64. Your patch just turn off build 32 bit overlay for other ARCHes. My patch keep place to do this. > Remove redundant USES=qt:5 instances. Is any "best practices" in handbook or in other "books" about this? I got build error in poudriere without: USES=qmake USE_QT=qmake:build Can test one more time… > Use CMake to install the files instead of using custom do-install target. During my tests it create files in ".build" dir and exit - no install in "stage". So I used old method with "custom do-install target". > BUILD_DEPENDS=microsoft-gsl>=3.0.0:devel/microsoft-gsl Didn't find it in ports before.
(In reply to Vladimir Druzenko from comment #4) Done. src/CMakeLists.txt: https://github.com/mumble-voip/mumble/pull/6191 overlay_gl/CMakeLists.txt: https://github.com/mumble-voip/mumble/pull/6192 (In reply to Vladimir Druzenko from comment #7) >> Only install cross-complied libraries on amd64. > Your patch just turn off build 32 bit overlay for other ARCHes. > My patch keep place to do this. I think this should be integrated into the build system then. Upstream currently only has provisions for installing the 32-bit library for amd64. Other archs just get the native library, but I am not sure how useful the 32-bit library is on 64-bit systems anyway. How many people are running 32-bit applications on 64-bit architectures? >> Remove redundant USES=qt:5 instances. > Is any "best practices" in handbook or in other "books" about this? Once it has been added to USES, it doesn't need to be added again. It reduces Makefile clutter. > I got build error in poudriere without: > USES=qmake > USE_QT=qmake:build > Can test one more time… When using USES=cmake, qmake should NOT be in USES. It overrides important environment variables that will make the cmake install fail. Qmake should instead be added to USE_QT as a build depends, like I did in the patch. >> Use CMake to install the files instead of using custom do-install target. > During my tests it create files in ".build" dir and exit - no install in > "stage". So I used old method with "custom do-install target". Related to above. cmake and qmake together in USES don't mix well.
Created attachment 244298 [details] Keep possibility for other ARCHes to build 32bit overlay Check my patch. I made it based on your + several tweaks.
(In reply to Vladimir Druzenko from comment #9) I appreciate you wanting to support other architectures and that's something we should be striving for. However, the use cases for mumble overlay 32-bit are extremely small on FreeBSD, even for amd64, since it's primarily meant for compatibility with 32-bit games that can't be recompiled for the native architecture for whatever reason, but mainly due to being closed source. I'm not that knowledgeable about the gaming scene on FreeBSD unless you count PS4/PS5 as FreeBSD, but I'm fairly certain I can count the number of closed source games released for FreeBSD-powerpc with a closed fist. Regardless, it's still important to do some bare minimum testing. In PATCH_SITES, you have 2 undefined variables: GH_ACCOUNT and GH_PROJECT, so the fetch stage fails. Please just follow MASTER_SITES and use PORTNAME. https://github.com/mumble-voip/mumble/pull/6192 hasn't been merged either and based on suggestions from upstream, it would be better to make the install of the 32-bit library more generic. So that commit hash is subject to change when I get around to addressing that. It might be better to keep that as a local patch for now. USE_CXXSTD should be grouped with USES and the other USE_* variables. I failed to eliminate the blank line between USES and USE_* in my patch, but not sure why you added an extra blank line in between. OPTIONS_SUB should be grouped with the other OPTIONS_* variables, but honestly, I would just get rid of it and use PLIST_SUB exclusively, since almost all of the plist substitution is already happening after <bsd.port.options.mk>. It makes it easier to parse what's going on. It also best practice to quote the substitution, even for empty strings. For example, you have: .if ${PORT_OPTIONS:MOVERLAY_32BIT} . if ${ARCH} == amd64 PLIST_SUB+= OVERLAY_32BIT_SUFFIX=.x86 PLIST_SUB+= OVERLAY_NATIVE_SUFFIX=.x86_64 . endif .else PLIST_SUB+= OVERLAY_32BIT="@comment " PLIST_SUB+= OVERLAY_NATIVE_SUFFIX= .endif I would drop OPTIONS_SUB and do: .if ${PORT_OPTIONS:MOVERLAY_32BIT} . if ${ARCH} == amd64 PLIST_SUB+= OVERLAY_32BIT="" PLIST_SUB+= OVERLAY_32BIT_SUFFIX=".x86" PLIST_SUB+= OVERLAY_NATIVE_SUFFIX=".x86_64" . endif .else PLIST_SUB+= OVERLAY_32BIT="@comment " PLIST_SUB+= OVERLAY_NATIVE_SUFFIX="" .endif Now you have parity for the OVERLAY_32BIT substitution. I still think it's a bit much to add another option for this, though.
Created attachment 244317 [details] Keep possibility for other ARCHes to build 32bit overlay v4 1. I just keep possibility to easy add support for other ARCHes without overcomplication of the Makefile. 2. My bad - modified Makefile part by part and test after each step, but forgot to change this. Tested in poudriere on 13.2, 12.4 amd64 and i386, but patches was downloaded already and port build fine… Fixed in v4. 3. I know, but while we discuss other parts this pull request can be commited. If no - I'll replace it with patch in files/. 4. It's grouped already. Removed blank lines in v4. 5. I thought about OPTIONS_SUB, but then I decided to leave it. Dropped OPTIONS_SUB=yes in v4.
(In reply to Vladimir Druzenko from comment #11) The only note I have is to quote the substitutions in the PLIST_SUBs, as I mentioned and gave example of in comment #10. See: https://docs.freebsd.org/en/books/porters-handbook/book/#plist-sub It's just bizarre to see an assignment operator without anything after it in a Makefile without good reason, and while sometimes there is very good reason, a short comment after the assignment operator explaining why helps to quell the red flags that tend to shoot up in fellow committers minds.
Created attachment 244343 [details] Keep possibility for other ARCHes to build 32bit overlay v5 Done. Decided to keep commented record for 2nd patch in Makefile.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=08c1696fb18eac0edbbdc884b5c346e9699c27cc commit 08c1696fb18eac0edbbdc884b5c346e9699c27cc Author: Vladimir Druzenko <vvd@FreeBSD.org> AuthorDate: 2023-08-28 18:41:13 +0000 Commit: Vladimir Druzenko <vvd@FreeBSD.org> CommitDate: 2023-08-28 18:41:13 +0000 audio/mumble: switch from USE_GITHUB to pre-rolled release and fix build on ARCHes other than x86 1. Switch from USE_GITHUB to pre-rolled release. 2. Use CMake to install the files instead of using custom do-install target. 3. Unbundle devel/microsoft-gsl. 4. Fix build and keep possibility to build 32bit overlay on other ARCHes than x86. 5. "Pet portclippy". Ideas, patches and testing: fuz, jhale, pkubaj. PR: 273203 272881 Approved by: tcberner Differential Revision: https://reviews.freebsd.org/D41604 MFH: 2023Q3 audio/mumble/Makefile | 118 ++++++++------------- audio/mumble/distinfo | 20 +--- ...ary__files_run__scripts_mumble-overlay.in (new) | 32 ++++++ .../files/patch-overlay__gl_CMakeLists.txt (new) | 16 +++ audio/mumble/pkg-plist | 19 ++-- 5 files changed, 107 insertions(+), 98 deletions(-)
A commit in branch 2023Q3 references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=3553d5df611e8c79350784060404ae643e9c85ba commit 3553d5df611e8c79350784060404ae643e9c85ba Author: Vladimir Druzenko <vvd@FreeBSD.org> AuthorDate: 2023-08-28 18:41:13 +0000 Commit: Vladimir Druzenko <vvd@FreeBSD.org> CommitDate: 2023-08-28 19:09:50 +0000 audio/mumble: switch from USE_GITHUB to pre-rolled release and fix build on ARCHes other than x86 1. Switch from USE_GITHUB to pre-rolled release. 2. Use CMake to install the files instead of using custom do-install target. 3. Unbundle devel/microsoft-gsl. 4. Fix build and keep possibility to build 32bit overlay on other ARCHes than x86. 5. "Pet portclippy". Ideas, patches and testing: fuz, jhale, pkubaj. PR: 273203 272881 Approved by: tcberner Differential Revision: https://reviews.freebsd.org/D41604 MFH: 2023Q3 (cherry picked from commit 08c1696fb18eac0edbbdc884b5c346e9699c27cc) audio/mumble/Makefile | 118 ++++++++------------- audio/mumble/distinfo | 20 +--- ...ary__files_run__scripts_mumble-overlay.in (new) | 32 ++++++ .../files/patch-overlay__gl_CMakeLists.txt (new) | 16 +++ audio/mumble/pkg-plist | 19 ++-- 5 files changed, 107 insertions(+), 98 deletions(-)
Thanks.