Page MenuHomeWildfire Games

Use pkg-config instead of hard-coding or library-specific programs
ClosedPublic

Authored by s0600204 on Aug 16 2018, 12:11 AM.

Details

Reviewers
Itms
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22302: Use pkg-config where possible instead of hard-coding or library-specific…
Trac Tickets
#5157
Summary

With continued problems in #5157, this time with regards to Slackware Linux, this revision expands on the scope of D1582 and tries to use pkg-config where possible.

For example, the X11 include-paths had been hard-coded as a list of possible locations, some of which existed on some distros, but not on others. This then presented a problem when some of those paths were soft-links to other paths, ie. /usr/X11R6/include is equivalent to /usr/include on Slackware.

As another example, gloox has its own config program: gloox-config. When run with the appropriate argument (--cflags) this was returning /usr/include, a directory that is included by default by gcc anyway and, as mentioned in D1582, a directory that if included too early causes problems.

In both instances, and in fact all instances below, a call to pkg-config has been used instead where possible. Unlike the hard-coding and (most) library-specific programs, pkg-config appears to take into account system headers included by gcc, thus preventing over-inclusion of certain files.

Depends on D1582


An earlier version of this changeset was confirmed to permit successful compilation on Slackware-current by @andy5995 over at #5157

Test Plan
  • As with its dependency D1582, this should be tested on various Linux distros. (And on BSD if anyone uses that...)
  • It should probably be tested on OSX, just to make sure there's been no breakages.
  • It should not need testing on Windows as there is no change to the logic-path used for that system.

Run make clean, update-workspaces.sh, then make in the appropriate folders.

Running clean-workspaces.sh should not be necessary.

Please report linux distro name & version, gcc version, and whether you're using the premake4 or premake5 script.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

s0600204 created this revision.Aug 16 2018, 12:11 AM
s0600204 created this object with visibility "s0600204".
s0600204 created this object with edit policy "s0600204".
Owners added a subscriber: Restricted Owners Package.Aug 16 2018, 12:11 AM
s0600204 updated this revision to Diff 6855.Aug 16 2018, 6:09 PM
s0600204 edited the summary of this revision. (Show Details)
s0600204 edited the test plan for this revision. (Show Details)
s0600204 changed the visibility from "s0600204" to "Public (No Login Required)".
s0600204 changed the edit policy from "s0600204" to "All Users".

Single-line most of the logic. Add icu support for the heck of it. Add explanatory comment to the wxwidgets section.

Vulcan added a subscriber: Vulcan.Aug 16 2018, 6:10 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/717/display/redirect

(arcanist seems to have lost the ability to apply dependencies before applying dependants.)

andy5995 added a comment.EditedAug 17 2018, 7:40 PM

I applied D1582 and then this patch (@s0600204 said it's dependent on D1582) and I got a complete build on Slackware64-current with gcc 8.2

Also builds on Debian 9 using gcc 6.3

Thanks for confirming that this works. Good to know!

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/718/display/redirect

I tested this patch using Travis..

https://travis-ci.org/andy5995/0ad/builds/417846934

So I can confirm it builds on Ubuntu trusty using gcc 4.8, 5, 6, 7, 8 and clang 5 and 6.

@Itms @s0600204 The ticket where this build failure is reported is on the a23 milestone and I'm not sure if this irrelevant enough to be pushed. There was another bugreport on Nov 20th on the lobby:

Maanyavar: 0ad doesnt work on slackware 14.2

Secondly this is also a gloox premake patch, so this might or might not influence D1654 and vice versa:

gloox has its own config program: gloox-config. When run with the appropriate argument (--cflags) this was returning /usr/include, a directory that is included by default by gcc anyway and, as mentioned in D1582, a directory that if included too early causes problems.

In D1611#66598, @elexis wrote:

Secondly this is also a gloox premake patch, so this might or might not influence D1654 and vice versa:

gloox has its own config program: gloox-config. When run with the appropriate argument (--cflags) this was returning /usr/include, a directory that is included by default by gcc anyway and, as mentioned in D1582, a directory that if included too early causes problems.

They'll no doubt be a merge conflict, but nothing further - the OSX build method continues to use the GLOOX_CONFIG override before and after both this and that patch, so changing other *nix-type systems to use pkg-config in this patch should have no effect on the logic of that patch.

s0600204 updated this revision to Diff 7044.Dec 12 2018, 10:08 PM

Rebase & update post-merging of D1654.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/822/

Itms requested changes to this revision.Dec 26 2018, 5:34 PM

This looks pretty cool, I'm eager to have this in!

Now after rP21955, you can remove the changes to the removed premake4 files.

I have two remarks, for each de-hardcoded library:

  • the and syntax is a bit confusing. I would not have understood it does a concatenation if I didn't know what we are trying to achieve. I think maybe we could change the pkgconfig module so that add_includes and add_libs take a third alternative_flags argument. If it is present, alternative_cmd is called with those flags, if it isn't, it is called with --cflags or --libs respectively.
  • Android is always left out. This does not come from the patch, but it makes it very apparent. Do you think moving that logic somewhere else would make sense? (most libraries must be ignored on Android, so it would be better to mark the ones that are needed, rather than the ones that must be ignored). Maybe leaving it like this would still be better, I don't know.
This revision now requires changes to proceed.Dec 26 2018, 5:34 PM
s0600204 updated this revision to Diff 7309.Jan 8 2019, 1:09 AM

Apply suggestions from @Itms

In D1611#67371, @Itms wrote:
  • Android is always left out. This does not come from the patch, but it makes it very apparent. Do you think moving that logic somewhere else would make sense? (most libraries must be ignored on Android, so it would be better to mark the ones that are needed, rather than the ones that must be ignored). Maybe leaving it like this would still be better, I don't know.

Hmm. To be honest, I may have been a little over-zealous with adding if not _OPTIONS["android"]. I've gone through and removed all that I added where there wasn't one before.

I don't think moving the logic elsewhere makes much sense - this file is used for working out how to include dependencies for all other OSes, so having it "all OSes... except Android which is handled over there" seems wrong somehow (even if "there" is elsewhere within the same file). Lets be honest, the build system is confusing enough without adding further special-casing 😉 (particularly for something that isn't really a priority, nor has seen much attention (from a development perspective) for four years).

Vulcan added a comment.Jan 8 2019, 2:20 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lib/sysdep/os/unix/x/x.cpp
|   1| /*·Copyright·(C)·2013·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2013"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/945/

Stan added a subscriber: Stan.EditedWed, May 8, 7:57 PM

@Itms do you have any other comments on this ?

Itms added a subscriber: wraitii.Tue, May 21, 8:13 PM

@wraitii Would you have time to test this on a mac?

Probably will at some point this WE. Be sure to remind me if I forget

Itms accepted this revision.Thu, May 23, 10:57 PM

This is working on my macOS VM and on Linux, and the code looks perfect. 👍

build/premake/extern_libs5.lua
644 ↗(On Diff #7309)

Maybe the comment from line 634 could be duplicated here, dunno.

This revision is now accepted and ready to land.Thu, May 23, 10:57 PM
wraitii accepted this revision.Sat, May 25, 2:53 PM

Compiles on my system.

This revision was automatically updated to reflect the committed changes.