Created attachment 184555 [details] patch QAs: * Builds in poudriere * passes portlint
A commit references this bug: Author: jbeich Date: Sat Jul 22 12:02:25 UTC 2017 New revision: 446390 URL: https://svnweb.freebsd.org/changeset/ports/446390 Log: multimedia/assimp: update to 4.0.0 Changes: https://github.com/assimp/assimp/releases/tag/v4.0.0 PR: 220889 Submitted by: Yuri Victorovich <yuri@rawbw.com> (maintainer) Changes: head/graphics/qt5-3d/Makefile head/multimedia/assimp/Makefile head/multimedia/assimp/distinfo head/multimedia/assimp/files/ head/multimedia/assimp/pkg-plist
Sorry, I didn't notice assignee.
A commit references this bug: Author: jbeich Date: Sat Jul 22 12:17:52 UTC 2017 New revision: 446394 URL: https://svnweb.freebsd.org/changeset/ports/446394 Log: games/pioneer: unbreak after r446390 checking for ASSIMP... yes checking for ASSIMP version >= 3.2... no configure: error: "assimp version >= 3.2 not found. Get it from https://assimp.sourceforge.net/" ===> Script "configure" failed unexpectedly. PR: 220889 Pointy hat to: jbeich Obtained from: upstream Approved by: portmgr blanket Changes: head/games/pioneer/Makefile head/games/pioneer/distinfo
A commit references this bug: Author: jbeich Date: Sat Jul 22 12:22:33 UTC 2017 New revision: 446395 URL: https://svnweb.freebsd.org/changeset/ports/446395 Log: multimedia/assimp: rebuild consumers after r446390 ABI: https://abi-laboratory.pro/tracker/timeline/assimp/ PR: 220889 Pointy hat to: jbeich Changes: head/games/doomsday/Makefile head/games/pioneer/Makefile head/graphics/qt5-3d/Makefile
$ uname -srm FreeBSD 11.0-RELEASE-p9 amd64 $ freebsd-version -k -u 11.0-RELEASE-p9 11.0-RELEASE-p11 Build errors: =============================================== [1] code/D3MFOpcPackage.cpp:221:16: error: use of undeclared identifier 'malloc' m_Buffer = malloc(m_Size); ^ code/D3MFOpcPackage.cpp:225:5: error: use of undeclared identifier 'free' free(m_Buffer); ^ 2 errors generated. Workaround: add #include <stdlib.h> =============================================== [2] In file included from code/MMDImporter.cpp:45: code/MMDPmdParser.h:161:14: error: cast from pointer to smaller type 'char' loses information *pstar = (char)NULL; ^~~~~~~~~~ 1 error generated. Workaround: replace on *pstar = (char)0; =============================================== [3] /usr/bin/ld: cannot find -lminizip c++: error: linker command failed with exit code 1 (use -v to see invocation) But I have minizip installed… Tired to find workarounds… ===============================================
The patch wasn't committed properly. It has 2 patches under files/ it has to leave: files/patch-code_MMDPmdParser.h files/patch-code_D3MFOpcPackage.cpp
Reopening.
[3] Workaround: in file /usr/ports/multimedia/assimp/work/assimp-4.0.0/build.ninja replace 3 entries of "-lminizip" with "-L/usr/local/lib -lminizip". After all 3 fixes multimedia/assimp builded without errors and dependent port graphics/qt5-3d too.
A commit references this bug: Author: jbeich Date: Sat Jul 22 19:33:40 UTC 2017 New revision: 446429 URL: https://svnweb.freebsd.org/changeset/ports/446429 Log: multimedia/assimp: add patches for 4.0.0 forgotten in r446390 PR: 220889 Reported by: vvd@unislabs.com Pointy hat to: jbeich Submitted by: Yuri Victorovich <yuri@rawbw.com> (maintainer) Changes: head/multimedia/assimp/files/ head/multimedia/assimp/files/patch-code_D3MFOpcPackage.cpp head/multimedia/assimp/files/patch-code_MMDPmdParser.h
(In reply to vvd from comment #5) Can you re-try and report if any more issues are still left?
(In reply to vvd from comment #5) > /usr/bin/ld: cannot find -lminizip assimp tries to find system minizip or falls back to bundled copy otherwise. When linking it uses _LIBRARIES variable which just contain library names rather _LDFLAGS. Both are supported by target_link_libraries(). There're 2 ways to work around the issue: either pass -DUNZIP_LIBRARIES=${LOCALBASE}/lib/libminizip.so via CMAKE_ARGS or add USES=localbase:ldflags. Either way PORTREVISION has to be bumped after the fix. Looks like bug 208563 resurfaced after 4.0.0 dropped post-patch.
/usr/bin/ld: cannot find -lminizip c++: error: linker command failed with exit code 1 (use -v to see invocation) ninja: build stopped: subcommand failed. Need patch [3] from my list.
vvd@unislabs.com If you install archivers/minizip, does it find it ok?
Created attachment 184602 [details] patch-adding-minizip-dependency Jan, Could you please add this patch? Somehow it's built fine for me without this dependency. Thanks! Yuri
(In reply to Yuri Victorovich from comment #13) As I said in comment #5 I have minizip installed.
(In reply to vvd from comment #15) Then this is a bug elsewhere. It has to find it once it is is installed.
Does adding > LDFLAGS+= -L${LOCALBASE}/lib gelp?
(In reply to Yuri Victorovich from comment #17) With > LDFLAGS+= -L${LOCALBASE}/lib in /usr/ports/multimedia/assimp/Makefile work fine.
Created attachment 184604 [details] patch-adding-minizip-dependency-and-LDFLAGS
Comment on attachment 184604 [details] patch-adding-minizip-dependency-and-LDFLAGS Convert to USES=localbase:ldflags instead and bump PORTREVISION.
(In reply to Jan Beich from comment #20) I can confirm, that USES=localbase:ldflags works on recent HEAD amd64.
Created attachment 184685 [details] The files/patch-CMakeLists.txt for minizip Hello. (In reply to comment #21) I can also confirm, that what proposed in attachment #184604 [details] or proposal from Jan Beich (in comment #20) works on FreeBSD 10.3. The alternative method is to add following code to CMakeLists.txt file after "use_pkgconfig(UNZIP minizip)": -8<-- IF ( UNZIP_FOUND ) LINK_DIRECTORIES(${UNZIP_LIBRARY_DIRS}) ENDIF( UNZIP_FOUND ) -->8-- Attached simple patch. Another alternative with using sed patch (with additional changes): -8<-- post-patch: # Remove unnecessary execution of git commands, when local repository isn't # available in distfile. Add location of minizip library for linker. @${REINPLACE_CMD} -e '/working branch$$/,/^)/d ; \ /minizip/s|$$|\${.newline}IF ( UNZIP_FOUND )\${.newline} LINK_DIRECTORIES($${UNZIP_LIBRARY_DIRS})\${.newline}ENDIF()|' \ ${WRKSRC}/CMakeLists.txt -->8- If usage of link_directories isn't advisable, there is a need to use absolute paths of needed libraries for target_link_libraries command (which maybe related to use_pkgconfig (from assimp) and/or pkg_check_modules macros (from CMake)). But I guess, this is not a case.
Created attachment 184686 [details] Proposed patch for games/pioneer (since 446395 revision) (In reply to comment #3) Also I created patch for games/pioneer port with following changes: - Replace PATCHFILES with simple sed patch, which doesn't require to download additional patch - Fix portlint warning about USES
Comment on attachment 184685 [details] The files/patch-CMakeLists.txt for minizip (In reply to lightside from comment #22) > Attached simple patch. It's less simple than USES=localbase:ldflags and may cause churn on updates. Better send the patch upstream. pkg_check_modules() is broken by design as _LIBRARIES don't use absolute paths thus have to be wrapped. The issue is skirted on Linux systems because default compiler there respects where package system installs headers/libraries by default.
A commit references this bug: Author: jbeich Date: Tue Jul 25 13:00:13 UTC 2017 New revision: 446581 URL: https://svnweb.freebsd.org/changeset/ports/446581 Log: multimedia/assimp: unbundle minizip The port prefers system minizip if available but the build fails due to an inconsitency between find_package() and pkg_check_modules() about whether _LIBRARIES should contain absolute paths. /usr/bin/ld: cannot find -lminizip c++: error: linker command failed with exit code 1 (use -v to see invocation) https://gitlab.kitware.com/cmake/cmake/issues/15804 PR: 220889 Reported by: vvd@unislabs.com Submitted by: yuri@rawbw.com (maintainer, based on) Changes: head/multimedia/assimp/Makefile
Comment on attachment 184686 [details] Proposed patch for games/pioneer (since 446395 revision) The proposed changes deal with style-only thus outside of scope for this bug. Please, file another bug and maybe refine rationale. > - Fix portlint warning about USES Porter's Handbook is ambigous how to sort USE_GITHUB if it's separated into its own block from the rest of USE_* variables. > - Replace PATCHFILES with simple sed patch, which doesn't require to download PATCHFILES highlights the fix was backported *as is* from upstream. On update there will be a conflict, so maintainer can quickly notice and drop it. > + /aiGetVersionMajor()/s| &&.*2||' \ Likely to turn into cruft if forgotten. After a53fdbe8b046 the change would break assimp < 4.0.0 case. It's always false in the ports tree except for unsupported partial upgrades.
All work fine. Thanks!
(In reply to comment #24) > It's less simple than USES=localbase:ldflags and may cause churn on updates. It was more intended for upstream, while I showed another (position independent) version with sed patch. Previous post-patch also worked (with possible removal of "s|$${ASSIMP_LIB_INSTALL_DIR}/pkgconfig|libdata/pkgconfig|" part), as you mentioned in comment #11. But looks like, the assimp's maintainer wanted newer minizip version from ports, probably, because of fixes. (In reply to comment #26) > The proposed changes deal with style-only thus outside of scope for this bug. > Please, file another bug and maybe refine rationale. This PR was mentioned in ports r446394 as the source of related patches. This is why it's here. (In reply to comment #26) > Porter's Handbook is ambigous how to sort USE_GITHUB if it's separated into > its own block from the rest of USE_* variables. I agree, that USE_GITHUB is more related to MASTER_SITES, which is before any USES. But portlint was changed to check this, as request to bug 220340. But this didn't stop other committer(s) to think otherwise and commit related changes (without question for maintainer), e.g. in ports r445407. (In reply to comment #26) > PATCHFILES highlights the fix was backported *as is* from upstream. On update > there will be a conflict, so maintainer can quickly notice and drop it. I already noticed this and dropped it with correct fix, in my opinion. The rationaly was explained in changes: - Replace PATCHFILES with simple sed patch, which doesn't require to download additional patch. (In reply to comment #26) > Likely to turn into cruft if forgotten. After a53fdbe8b046 the change would > break assimp < 4.0.0 case. It's always false in the ports tree except for > unsupported partial upgrades. The proposed patch turns following check from if (aiGetVersionMajor() >= 3 && aiGetVersionMinor() >= 2) to if (aiGetVersionMajor() >= 3) which works for previous 3.3.1 and newer 4.0.0 version of assimp (e.g. >= 3). It's just more simple version of the changes, which also works for newer pioneer version after a53fdbe8b046 commit and didn't break the build (even if someone decide to update on their own to check development version(s), because there is still check for "aiGetVersionMajor() == 3" after "aiGetVersionMajor() > 3"). So, please commit and mention this PR, as you did for ports r446394. The suggested descriptions of changes was already proposed. The PORTREVISION bump isn't required. If this will be Jan Beich, then you can ignore the changes for USES, but I think, that better to propose this on my own, than reading longer descriptions for style changes in commit logs, if some committer will decide to propose this massive update for affected ports.
Comment on attachment 184686 [details] Proposed patch for games/pioneer (since 446395 revision) Proposed patch for games/pioneer removed after ports r451387 update.