Page MenuHomeFreeBSD

multimedia/gxine: Remove dependency that no longer exists, other cleanup
ClosedPublic

Authored by ndowens_yahoo.com on Sep 1 2018, 12:19 AM.

Details

Summary

Remove dependency that no longer exists, and may not be needed
Changed a few OPTION switches
General clean up

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Updating D16978: multimedia/gxine
Add NLS knob to remove gettext as a must dep

linimon retitled this revision from multimedia/gxine to multimedia/gxine: Remove dependency that no longer exists, other cleanup.Sep 1 2018, 5:29 AM
mat added inline comments.Sep 1 2018, 8:21 AM
multimedia/gxine/Makefile
26 ↗(On Diff #47547)

This is probably a bad idea.

27–28 ↗(On Diff #47547)

Most of this would be USES=localbase

34–35 ↗(On Diff #47547)

Part of this should be:

GNOME_CONFIGURE_WITH=  dbus
40 ↗(On Diff #47547)

DESCs should happen earlier, See Chapter 15. Order of Variables in Port Makefiles.

57–58 ↗(On Diff #47547)

You could probably fold those two commands into one.

tobik added inline comments.Sep 1 2018, 9:28 AM
multimedia/gxine/Makefile
14 ↗(On Diff #47547)

LICENSE_FILE (no S at the end)

26 ↗(On Diff #47547)

I feel like this is a problem that must be fixed in libxine and not worked around in every consumer.

I don't understand why compiling gxine with -fPIC fixes linking to libxine with lld.

28 ↗(On Diff #47547)

I don't think -lm is needed anymore.

33–35 ↗(On Diff #47547)

I am wondering if this does anything without GNOME screensaver running or if it is useless without it?

--without-dbus          disable dbus support (for GNOME screensaver)
56–58 ↗(On Diff #47547)

logo.jpg is already being installed. No need to do it manually. gxine now uses logo.jpg since the libxine update, so logo.mpv is not needed anymore. Let's drop post-install completely.

ndowens_yahoo.com marked 8 inline comments as done.Sep 1 2018, 10:31 AM
ndowens_yahoo.com added inline comments.
multimedia/gxine/Makefile
26 ↗(On Diff #47547)

Ed Maste replied to what I figured out that fPIC works around linker issue
@tobik @mat
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=230994

LLDunsafe didn’t work nor adding the ldflags Ed said is medium

27–28 ↗(On Diff #47547)

Was thinking about removing before posting here but didn’t know what -1m was for

33–35 ↗(On Diff #47547)

I have no idea myself. I don’t use gnome and I don’t understand how a media player would even need screensaver dep. IIRC when I use to use gnome stuff I felt as gnome apps relied on dbus, but I could be wrong

ndowens_yahoo.com marked an inline comment as done.Sep 1 2018, 10:43 AM
ndowens_yahoo.com updated this revision to Diff 47560.

Updating D16978: multimedia/gxine: Remove dependency that no longer exists, other cleanup
Fix typo in license file
remove cpp and ldflags
remove post-install block

tobik added inline comments.Sep 1 2018, 10:50 AM
multimedia/gxine/Makefile
26 ↗(On Diff #47547)

LLDunsafe didn’t work

?

Of course LLD_UNSAFE works here or we wouldn't have been able to build gxine on 12.0/amd64 at all previously.

multimedia/gxine/Makefile
26 ↗(On Diff #47547)

Retying but I do believe I was getting linker issues with it

tobik added inline comments.Sep 1 2018, 10:56 AM
multimedia/gxine/Makefile
33–35 ↗(On Diff #47547)

I say remove it in that case. The option has been broken for 4 years (when gnome-screensaver was removed) and nobody has complained so I doubt anybody cares about it.

When you remove it make sure to append --disable-integration-wizard --without-dbus to CONFIGURE_ARGS in lieu of GNOME_CONFIGURE_OFF.

ndowens_yahoo.com marked 5 inline comments as done.Sep 1 2018, 11:34 AM

Updating D16978: multimedia/gxine: Remove dependency that no longer exists, other cleanup
Re-add lld_unsafe
Remove gnome option

ndowens_yahoo.com abandoned this revision.Sep 5 2018, 4:58 AM
tobik added a reviewer: tobik.Sep 5 2018, 5:08 AM

Why did you abandon this?

ndowens_yahoo.com reclaimed this revision.Sep 5 2018, 5:16 AM

Why did you abandon this?

For some reason, thought it was commited along with the update with libxine :) But reclaimed

tobik accepted this revision.Sep 5 2018, 5:22 AM
This revision is now accepted and ready to land.Sep 5 2018, 5:22 AM

@tobik As you may know, I can't commit :)

This revision was automatically updated to reflect the committed changes.
tobik added a comment.Sep 7 2018, 5:21 PM

@tobik As you may know, I can't commit :)

Yeah, I know. Please do not ping me after only a couple of days though. The review is barely even a week old and hardly urgent.

In general it's also a good idea to open a corresponding bug on Bugzilla for every review, so that others can take care of it too. Phabricator is not a patch queue.