Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pangolin: fix dependencies list #1363

Merged
merged 1 commit into from Mar 6, 2018
Merged

Pangolin: fix dependencies list #1363

merged 1 commit into from Mar 6, 2018

Conversation

ex-purple
Copy link
Contributor

Description

https://trac.macports.org/ticket/55924

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 10.13.3
Xcode 9.2

Verification

Have you

  • checked your Portfile with port lint?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?

mf2k
mf2k requested changes Mar 1, 2018
View reviewed changes
graphics/Pangolin/Portfile Outdated
port:pkgconfig

depends_lib-append port:glew \
port:ffmpeg \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be updated to allow ffmpeg-devel to be used also.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ryandesign
Copy link
Contributor

Since this changes how the port will build and what it will install, the revision needs to be increased.

Since this is meant to fix a ticket, a second paragraph "Closes: https://trac.macports.org/ticket/55924" should be in the commit message.

These changes are insufficient to address all of the issues mentioned in the ticket. What about python, tiff, libpng, jpeg, zlib?

@ryandesign
Copy link
Contributor

These changes are insufficient to address all of the issues mentioned in the ticket. What about python, tiff, libpng, jpeg, zlib?

Unless -DBUILD_PANGOLIN_GUI=OFF takes care of that; I didn't test your changes yet.

@ex-purple
Copy link
Contributor Author

-DBUILD_PANGOLIN_GUI=OFF takes care of python. ffmpeg depends on tiff, libpng, jpeg, zlib

@ryandesign
Copy link
Contributor

-DBUILD_PANGOLIN_GUI=OFF takes care of python.

Ok.

ffmpeg depends on tiff, libpng, jpeg, zlib

Pangolin itself links with tiff, libpng, jpeg, zlib, so Pangolin should itself declare dependencies on them. One of the purposes of declaring dependencies is so that we can know just by using grep on the portfiles which ports link with which libraries, so that if one of those libraries is updated to a new library version (such as when libpng was updated from 1.2.x to 1.4.x, and again when it was updated from 1.4.x to 1.6.x) we will know which ports will need to be rebuilt against that new library version so that we can increase their revision.

ryandesign
ryandesign reviewed Mar 2, 2018
View reviewed changes
graphics/Pangolin/Portfile Outdated
path:lib/libavcodec.dylib:ffmpeg \
path:lib/libavformat.dylib:ffmpeg \
path:lib/libavutil.dylib:ffmpeg \
path:lib/libswscale.dylib:ffmpeg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A dependency on a port should only be added once. Most ports that depend on ffmpeg do so using path:lib/libavcodec.dylib:ffmpeg so that's the one I'd use here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

mf2k
mf2k reviewed Mar 2, 2018
View reviewed changes
graphics/Pangolin/Portfile
port:libpng \
port:jpeg \
port:tiff \
path:lib/libavcodec.dylib:ffmpeg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the incantation we normally use in other ports:
path:bin/ffmpeg:ffmpeg

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ports that use the ffmpeg binary should use path:bin/ffmpeg:ffmpeg. (There are currently 3 ports doing so.) This port uses the ffmpeg libraries, so it should use path:lib/libavcodec.dylib:ffmpeg. (There are currently 103 ports doing so.)

Copy link
Contributor Author

@ex-purple ex-purple Mar 2, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mf2k, but pangolin is not associated with ffmeg application, it only requires libraries. For example opencv use path:lib/libavcodec.dylib:ffmpeg

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well whatever Ryan thinks is best is good with me! :)

@pmetzger
Copy link
Member

pmetzger commented Mar 3, 2018

@ierofant You still need to bump the revision.

@mf2k @ryandesign If the revision number is bumped, will this be ready to merge?

@ex-purple
Pangolin: fix dependencies list
25e2681 
Closes: https://trac.macports.org/ticket/55924

* append ffmpeg, openexr, libdc1394, zlib, libpng, jpeg, tiff to depends_lib
* disable pangolin gui
* bump revison
@ex-purple
Copy link
Contributor Author

Revision is bumped

@pmetzger
Copy link
Member

pmetzger commented Mar 6, 2018

Okay, I am assuming this is ready to merge, so I'm going to. Thank you for your contribution to MacPorts!

@pmetzger pmetzger merged commit 6d30eb4 into macports:master Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryandesign ryandesign ryandesign left review comments

@mf2k mf2k mf2k approved these changes

Assignees
No one assigned
Labels
maintainer: none type: bugfix
Milestone
No milestone
5 participants
@ex-purple @ryandesign @pmetzger @mf2k @macportsbot