Bug 220889 - multimedia/assimp: Update to 4.0.0
Summary: multimedia/assimp: Update to 4.0.0
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Jan Beich
URL:
Keywords: patch, patch-ready
Depends on:
Blocks:
 
Reported: 2017-07-20 19:03 UTC by Yuri Victorovich
Modified: 2017-10-06 15:29 UTC (History)
4 users (show)

See Also:


Attachments
patch (7.43 KB, patch)
2017-07-20 19:03 UTC, Yuri Victorovich
yuri: maintainer-approval+
Details | Diff
patch-adding-minizip-dependency (437 bytes, patch)
2017-07-22 20:41 UTC, Yuri Victorovich
yuri: maintainer-approval+
Details | Diff
patch-adding-minizip-dependency-and-LDFLAGS (664 bytes, patch)
2017-07-22 22:03 UTC, Yuri Victorovich
no flags Details | Diff
The files/patch-CMakeLists.txt for minizip (338 bytes, patch)
2017-07-25 03:48 UTC, lightside
no flags Details | Diff
Proposed patch for games/pioneer (since 446395 revision) (1.42 KB, patch)
2017-07-25 03:51 UTC, lightside
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuri Victorovich freebsd_committer 2017-07-20 19:03:53 UTC
Created attachment 184555 [details]
patch

QAs:
* Builds in poudriere
* passes portlint
Comment 1 commit-hook freebsd_committer 2017-07-22 12:03:30 UTC
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
Comment 2 Jan Beich freebsd_committer 2017-07-22 12:04:25 UTC
Sorry, I didn't notice assignee.
Comment 3 commit-hook freebsd_committer 2017-07-22 12:18:49 UTC
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
Comment 4 commit-hook freebsd_committer 2017-07-22 12:22:55 UTC
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
Comment 5 VVD 2017-07-22 18:39:53 UTC
$ 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…
===============================================
Comment 6 Yuri Victorovich freebsd_committer 2017-07-22 19:12:40 UTC
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
Comment 7 Yuri Victorovich freebsd_committer 2017-07-22 19:13:13 UTC
Reopening.
Comment 8 VVD 2017-07-22 19:27:11 UTC
[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.
Comment 9 commit-hook freebsd_committer 2017-07-22 19:33:46 UTC
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
Comment 10 Jan Beich freebsd_committer 2017-07-22 19:46:15 UTC
(In reply to vvd from comment #5)
Can you re-try and report if any more issues are still left?
Comment 11 Jan Beich freebsd_committer 2017-07-22 20:12:12 UTC
(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.
Comment 12 VVD 2017-07-22 20:21:05 UTC
/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.
Comment 13 Yuri Victorovich freebsd_committer 2017-07-22 20:36:44 UTC
vvd@unislabs.com

If you install archivers/minizip, does it find it ok?
Comment 14 Yuri Victorovich freebsd_committer 2017-07-22 20:41:26 UTC
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
Comment 15 VVD 2017-07-22 21:07:27 UTC
(In reply to Yuri Victorovich from comment #13)
As I said in comment #5 I have minizip installed.
Comment 16 Yuri Victorovich freebsd_committer 2017-07-22 21:22:51 UTC
(In reply to vvd from comment #15)

Then this is a bug elsewhere. It has to find it once it is is installed.
Comment 17 Yuri Victorovich freebsd_committer 2017-07-22 21:24:31 UTC
Does adding
> LDFLAGS+=	-L${LOCALBASE}/lib
gelp?
Comment 18 VVD 2017-07-22 21:57:55 UTC
(In reply to Yuri Victorovich from comment #17)

With
> LDFLAGS+=	-L${LOCALBASE}/lib
in /usr/ports/multimedia/assimp/Makefile work fine.
Comment 19 Yuri Victorovich freebsd_committer 2017-07-22 22:03:13 UTC
Created attachment 184604 [details]
patch-adding-minizip-dependency-and-LDFLAGS
Comment 20 Jan Beich freebsd_committer 2017-07-23 03:39:55 UTC
Comment on attachment 184604 [details]
patch-adding-minizip-dependency-and-LDFLAGS

Convert to USES=localbase:ldflags instead and bump PORTREVISION.
Comment 21 Rainer Hurling 2017-07-23 11:16:57 UTC
(In reply to Jan Beich from comment #20)

I can confirm, that USES=localbase:ldflags works on recent HEAD amd64.
Comment 22 lightside 2017-07-25 03:48:43 UTC
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.
Comment 23 lightside 2017-07-25 03:51:00 UTC
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 24 Jan Beich freebsd_committer 2017-07-25 13:00:13 UTC
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.
Comment 25 commit-hook freebsd_committer 2017-07-25 13:00:41 UTC
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 26 Jan Beich freebsd_committer 2017-07-25 13:00:42 UTC
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.
Comment 27 VVD 2017-07-25 13:39:11 UTC
All work fine.

Thanks!
Comment 28 lightside 2017-07-25 14:37:19 UTC
(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 29 lightside 2017-10-06 15:29:34 UTC
Comment on attachment 184686 [details]
Proposed patch for games/pioneer (since 446395 revision)

Proposed patch for games/pioneer removed after ports r451387 update.