Opened 11 years ago
Closed 10 years ago
#43579 closed submission (fixed)
zathura-plugin-pdf-poppler @0.2.5
Reported by: | harciga | Owned by: | macports-tickets@… |
---|---|---|---|
Priority: | Normal | Milestone: | |
Component: | ports | Version: | 2.2.1 |
Keywords: | Cc: | ||
Port: | zathura-plugin-pdf-poppler |
Description
new port
Attachments (10)
Change History (21)
Changed 11 years ago by harciga
Changed 11 years ago by harciga
Attachment: | patch-Makefile added |
---|
Changed 11 years ago by harciga
Attachment: | patch-config.mk added |
---|
comment:1 Changed 11 years ago by harciga
comment:2 follow-ups: 4 5 Changed 11 years ago by ryandesign (Ryan Carsten Schmidt)
Thanks. Some observations about the Portfile:
- There should be a blank line between the "
# $Id$
" line and the "PortSystem 1.0
" line; running "port lint --nitpick
" should mention that. - The license field should be just "
zlib
", not "zlib License
" (the license field is a space-separated list of values; consult the port_binary_distributable.tcl script for a non-exhaustive list of license names) - Checksums are not used for patchfiles that will be copied to the files directory; they are used for patchfiles that get downloaded, but we only do that in the very unusual circumstance that a patchfile is provided by a third party and it is too large to be conveniently copied into the files directory.
- The checksums line doesn't need to mention the distfile name, because there is only one file being checksummed.
- Patchfile names should be of the form "patch-*.diff", as "
port lint --nitpick
" should mention. - Because use you "
use_configure no
", you must add code to ensure you're UsingTheRightCompiler, build with-arch
flags, and if possible add a universal variant. - lib- and bin-style dependencies allow non-MacPorts software to satisfy them. We don't normally want that; see wiki:FAQ. So usually you should use port-style dependencies (if only a single port exists that can satisfy it), or path-style dependencies (if there are multiple ports that can satisfy it). Or if there is another reason why you used lib- and bin-style dependencies in this port, let me know.
Regarding patch-Makefile:
- The install_name of a library should be the final location of the library when it is installed; it should not begin with ${DESTDIR}
- The install_name should usually be set using the "
-install_name
" flag; is there a particular reason you used "-Wl,-dylib_install_name,
" instead?
comment:3 Changed 11 years ago by ryandesign (Ryan Carsten Schmidt)
Keywords: | pwmt removed |
---|---|
Summary: | zathura-plugin-pdf-poppler @0.2.7 → zathura-plugin-pdf-poppler @0.2.5 |
comment:4 Changed 11 years ago by harciga
Replying to ryandesign@…:
Thanks. Some observations about the Portfile:
I'll address these issues tomorrow, as well as in the other 2 dependant port submissions (zathura & girara)
comment:5 Changed 11 years ago by harciga
Replying to ryandesign@…:
Thanks. Some observations about the Portfile:
- There should be a blank line between the "
# $Id$
" line and the "PortSystem 1.0
" line; running "port lint --nitpick
" should mention that.
Yup, fixed the warnings. port lint
is mentioned at §3.1.18 lint https://guide.macports.org/index.html#using.port.lint, I missed it. Perhaps it should be reminded of at §4.7 Portfile Best Practices?
- The license field should be just "
zlib
", not "zlib License
" (the license field is a space-separated list of values; consult the port_binary_distributable.tcl script for a non-exhaustive list of license names)
Fixed, the guide at §5.1 Global keywords https://guide.macports.org/index.html#reference.keywords makes no mention of this, should I open a ticket?
- Checksums are not used for patchfiles that will be copied to the files directory; they are used for patchfiles that get downloaded, but we only do that in the very unusual circumstance that a patchfile is provided by a third party and it is too large to be conveniently copied into the files directory.
Removed. port distfiles PORTNAME
gives reason to think patchfiles are distfiles. From §5.3.4 Checksum Phase Keywords
At least two checksum types (e.g., rmd160 and sha256) should be used to ensure the integrity of the distfiles.
- The checksums line doesn't need to mention the distfile name, because there is only one file being checksummed.
Ok.
- Patchfile names should be of the form "patch-*.diff", as "
port lint --nitpick
" should mention.
Fixed.
- Because use you "
use_configure no
", you must add code to ensure you're UsingTheRightCompiler, build with-arch
flags, and if possible add a universal variant.
I have checked the Makefile and it uses ${CC}, I also added the recommended lines for Ports with nonstandard or non-existent configure scripts https://trac.macports.org/wiki/UsingTheRightCompiler#nonstandard-ports
-- build with -arch- flags
Is this not handled by default?
-- add a universal variant
Why? §5.3.7.1 Configure Universal
There is a default universal variant made available to all ports by MacPorts base, so redefining universal keywords should only be done to make a given port compile if the default options fail to do so.
- lib- and bin-style dependencies allow non-MacPorts software to satisfy them. We don't normally want that; see wiki:FAQ. So usually you should use port-style dependencies (if only a single port exists that can satisfy it), or path-style dependencies (if there are multiple ports that can satisfy it). Or if there is another reason why you used lib- and bin-style dependencies in this port, let me know.
Switched both lib: and bin: dependencies to path: style. OS X is not supported by pwnt.org nor alternative package managers, so only macports alternative -devel ports will satisfy the dependencies in the foreseeable future.
Regarding patch-Makefile:
- The install_name of a library should be the final location of the library when it is installed; it should not begin with ${DESTDIR}
It has nothing to do with §7.2.4 DESTDIR: Support for Staged Installs http://www.gnu.org/prep/standards/html_node/DESTDIR.html It's the smallest change I could think of to fix the dynamic library lookup path from relative to absolute. It's a nitpick, it works correctly either way, but since I'm fixing port lint
trailing whitespaces...
- The install_name should usually be set using the "
-install_name
" flag; is there a particular reason you used "-Wl,-dylib_install_name,
" instead?
man ld
line 323-324.
This option is also called -dylib_install_name for compatibility.
Changed 11 years ago by harciga
Attachment: | Portfile.2 added |
---|
Changed 11 years ago by harciga
Attachment: | patch-Makefile.diff added |
---|
Changed 11 years ago by harciga
Attachment: | patch-config.mk.diff added |
---|
comment:6 Changed 11 years ago by harciga
Amended a path style dependency, it works either way so I guess there's some sort of sorcery going on in there.
Changed 11 years ago by harciga
Attachment: | Portfile.3 added |
---|
Changed 11 years ago by harciga
Attachment: | Portfile.4 added |
---|
comment:8 Changed 11 years ago by harciga
Ok, added -arch
flags and variant universal
and tested the Portfile to work correctly.
Is there anything else I should address for approval?
Changed 11 years ago by harciga
Attachment: | Portfile.5 added |
---|
comment:9 follow-up: 10 Changed 11 years ago by harciga
This Portfile is not ready for submission, variant universal
is not building fat binaries and -arch
flags while it builds cleanly produces a misbehaving binary.
I understand variant universal
can be removed and worked at a later date if fat binaries are requested, but -arch
flags are a requisite for port approval because of the use_configure no
directive. I have to find out what is causing the bug.
Changed 10 years ago by harciga
Attachment: | Portfile.6 added |
---|
comment:10 Changed 10 years ago by harciga
universal variant
is working correctly now. And the application is behaving correctly.
$ lipo -info /opt/local/lib/zathura/pdf.dylib
Architectures in the fat file: /opt/local/lib/zathura/pdf.dylib are: x86_64 i386
Let me know if there is anything left to address.
comment:11 Changed 10 years ago by neverpanic (Clemens Lang)
Resolution: | → fixed |
---|---|
Status: | new → closed |
Committed in r120312 with the following changes:
- Added missing build dependency on
port:pkgconfig
. - Added
VERBOSE=1
tobuild.env
to simplify debugging build failures in the future. - Modified
patch-Makefile.diff
to link as a bundle (-bundle
) and specify the bundle loader (-Wl,-bundle_loader,${PREFIX}/bin/zathura
) to avoid undefined symbols. Removed library version numbers because those aren't supported by ld64 in bundle mode.
It's actually version 0.2.5, the latest. Wouldn't let me modify it.