Bug 231015 - games/crashtest: Use pkg-config to find libode, etc
Summary: games/crashtest: Use pkg-config to find libode, etc
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: Dmitry Marakasov
URL:
Keywords:
Depends on:
Blocks: 230359
  Show dependency treegraph
 
Reported: 2018-08-30 06:41 UTC by Yuri Victorovich
Modified: 2019-02-21 20:59 UTC (History)
0 users

See Also:
bugzilla: maintainer-feedback? (amdmi3)


Attachments
patch (1.75 KB, patch)
2018-08-30 06:41 UTC, Yuri Victorovich
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 2018-08-30 06:41:07 UTC
Created attachment 196696 [details]
patch

Changes:
* Use pkg-config to find libode
* Add LICENSE
* Add missing dependencies and use clauses
Comment 1 Dmitry Marakasov freebsd_committer 2018-08-30 14:00:04 UTC
New dependencies are invalid as they come from fltk, not this port.
Again, what's the purpose of using libode via pkgconfig?
Comment 2 Yuri Victorovich freebsd_committer 2018-08-30 15:28:09 UTC
(In reply to Dmitry Marakasov from comment #1)

> New dependencies are invalid as they come from fltk, not this port.

No, they all are reported by stage-qa, and stage-qa only reports dependencies that this port has.

> Again, what's the purpose of using libode via pkgconfig?

This port has a hard-coded libode.a which isn't reported by ode's `pkf-config --libs ode`. Consequently, when static libs are removed from ode, this port fails.
Comment 3 Dmitry Marakasov freebsd_committer 2018-08-30 15:54:25 UTC
(In reply to Yuri Victorovich from comment #2)
> (In reply to Dmitry Marakasov from comment #1)
> 
> > New dependencies are invalid as they come from fltk, not this port.
> 
> No, they all are reported by stage-qa, and stage-qa only reports
> dependencies that this port has.

stage-qa reports all libraries despite the port they are coming form, and is wrong in this case.
Adding dependencies of dependencies to the port would be totally incorrect.

> > Again, what's the purpose of using libode via pkgconfig?
> 
> This port has a hard-coded libode.a which isn't reported by ode's
> `pkf-config --libs ode`. Consequently, when static libs are removed from
> ode, this port fails.

Why would libode.a be removed from libode port in the first place?
Comment 4 Yuri Victorovich freebsd_committer 2018-08-30 16:00:27 UTC
(In reply to Dmitry Marakasov from comment #3)

> stage-qa reports all libraries despite the port they are coming form, and is wrong in this case.
> Adding dependencies of dependencies to the port would be totally incorrect.

No! stage-qa only reports dependencies of this specific port. It doesn't report them recursively. I wrote this check (proxydeps) in stage-qa, it is designed this way.


> Why would libode.a be removed from libode port in the first place?

It takes space both on disk and in dependent packages, while providing no benefit.
portmgr's policy is to remove static libraries where possible.
Comment 5 Dmitry Marakasov freebsd_committer 2018-08-30 16:32:02 UTC
(In reply to Yuri Victorovich from comment #4)

> No! stage-qa only reports dependencies of this specific port. It doesn't
> report them recursively. I wrote this check (proxydeps) in stage-qa, it is
> designed this way.

No. stage-qa has no way to know where these dependencies come from. In fact, they come from `fltk-config`. They are exposed fltk dependencies. crashtest do not use any of them directly, and knows nothing of them. They are not it's dependencies. Recording them as such would be incorrect and will bring a lot of pain if list of dependencies exposed by fltk changes.

> > Why would libode.a be removed from libode port in the first place?
> 
> It takes space both on disk and in dependent packages, while providing no
> benefit.

Static linking results is slightly faster code, which may be noticeable in physics engine.

> portmgr's policy is to remove static libraries where possible.

I'd like to hear from portmgr directly on this case. What actually worries me is the need to modify all consumers and maintain these patches forever for the sake of cosmetic change.
Comment 6 Yuri Victorovich freebsd_committer 2018-08-30 17:00:44 UTC
(In reply to Dmitry Marakasov from comment #5)

> In fact, they come from `fltk-config`. They are exposed fltk dependencies.

Yes, they are due to the bug in fltk-config that it exposes its own dependencies. I will report this bug to them.


> Static linking results is slightly faster code, which may be noticeable in physics engine.

This argument can be made about every single library. Following this path would cause lots of shared libraries installed on disk, and embedded into other executables.

There are other considerations than slight speed gains that have to be balanced here: ease of maintenance, disk space, ease of fixing security problems. Additionally, speed gain is only real when lots of calls are made into the library, which isn't clear that it is the case here.
Comment 7 Dmitry Marakasov freebsd_committer 2018-08-30 20:09:21 UTC
(In reply to Yuri Victorovich from comment #6)

> > In fact, they come from `fltk-config`. They are exposed fltk dependencies.
> 
> Yes, they are due to the bug in fltk-config that it exposes its own
> dependencies. I will report this bug to them.

It's not necessarily a bug. pkg-config (or alike) exposing secondary library dependencies is unfortunately pretty common pattern (for instance, it's used in most of gnome stuff) and may have its reasons, such as fltk headers referring the constructs of its dependencies. In such case it would make sense to register these dependencies, but it's still incorrect to hardcode them manually in every fltk consumer port. The correct solution in this case would be to create Uses/fltk.mk which automatically adds all the dependencies fltk-config exposes in addition to fltk dependency itself.

> > Static linking results is slightly faster code, which may be noticeable in physics engine.
> 
> This argument can be made about every single library. Following this path
> would cause lots of shared libraries installed on disk, and embedded into
> other executables.
> 
> There are other considerations than slight speed gains that have to be
> balanced here: ease of maintenance, disk space, ease of fixing security
> problems. Additionally, speed gain is only real when lots of calls are made
> into the library, which isn't clear that it is the case here.

I've just mentioned the performance argument as a reply to `no benefit` statement, I don't really consider neither the performance, nor the disk space arguments as viable for/against static libraries. Maintenance and security are though, but I doubt that maintenance cost of modifying ode consumers outweighs the benefits of not having static library here.
Comment 8 Yuri Victorovich freebsd_committer 2018-08-30 20:19:25 UTC
(In reply to Dmitry Marakasov from comment #7)

The disk space problem is incremental. If ode is used by 10 ports and static lib is used, it will be copied 10 times. If everything uses only static libs - the problem will be major.

The disk space increase will also cause a memory consumption growth problem, because this code will be copied into memory several times if several consumenrs of such library run simultaneously.

The performance degradation because of the cache mishits can easily outweigh the original slightly higher speed due to the use of static libs.

This is why I always remove static libs when I can: they are more of a problem than a solution.
Comment 9 Dmitry Marakasov freebsd_committer 2018-09-13 12:00:11 UTC
Anyway, please remove extra depends coming from fltk, and this would be acceptable.
Comment 10 commit-hook freebsd_committer 2019-02-21 20:53:40 UTC
A commit references this bug:

Author: amdmi3
Date: Thu Feb 21 20:52:59 UTC 2019
New revision: 493532
URL: https://svnweb.freebsd.org/changeset/ports/493532

Log:
  - Avoid static linking with ode

  PR:		231015
  Submitted by:	yuri@freebsd.org

Changes:
  head/games/crashtest/Makefile
  head/games/crashtest/files/patch-src-crashtest_Makefile