Refactor: Add libpacemaker-internal

[clumens]

This renames libpengine to libpacemaker-internal, moves it into lib/, does a lot of build system tweaking, and adds tools/fake_transition.c into the library. It seems pretty straightforward to me, except for the libpacemaker_internal_la_LIBADD block in lib/pacemaker-internal/Makefile.am. I'm not sure whether adding libtransitioner and liblrmd there is okay.

[kgaillot]

Awesome :)

[clumens]

I could start renaming functions to pcmki_ as well - this is an internal library, so I don't think we care about backwards compatibility at all here.

[kgaillot]

I could start renaming functions to pcmki_ as well - this is an internal library, so I don't think we care about backwards compatibility at all here.

We can, but I'd leave it for a later PR, since this is going to touch so many files already. It'll become a merge nightmare if we let it grow.

BTW with these changes, we no longer need -I$(top_srcdir)/daemons/schedulerd for the stuff that used libpengine.

Seeing the scheme in practice, I'm thinking one tweak, let me know what you think: Keep the subsidiary headers named pcmki_*.h but name the main header pacemaker-internal.h. From the application's perspective, it links against libwhatever and includes whatever.h; the prefixed files are helpful for us but hidden from them.

[clumens]

Seeing the scheme in practice, I'm thinking one tweak, let me know what you think: Keep the subsidiary headers named pcmki_*.h but name the main header pacemaker-internal.h. From the application's perspective, it links against libwhatever and includes whatever.h; the prefixed files are helpful for us but hidden from them.

I think that is reasonable, yes. I'll make the change and push a new patch.

[clumens]

I don't think this PR is waiting on anything, though it still says changes requested.

[kgaillot]

Whoops, we both missed one file: lib/pacemaker-pengine.pc.in (pkg-config configuration). The pkg-config man page describes the file format. This is the first time I've actually looked at it; it looks like more or less a copy of pacemaker.pc.in, and I see "sub" is wrong already, we really don't need most of the variables we declare, and we probably aren't using all the fields optimally. :-/

[clumens]

Looks like that might have just been copied from lib/pacemaker-cib.pc.in. It's been that way since 2012, when the file was added. So that's nice. Probably most everything in that file can go away and we can do something more like what's in pacemaker-service.pc.in. On the other hand, does this count as API we need to preserve?

[clumens]

I'm thinking something like this:

index f9db162c8..64fa7c5f7 100644
--- a/lib/pacemaker-pengine.pc.in
+++ b/lib/pacemaker-pengine.pc.in
@@ -1,22 +1,10 @@
-sub=cib
+sub=pengine
 prefix=@prefix@
 libdir=@libdir@
 includedir=@includedir@/@PACKAGE_TARNAME@
 
 build=@BUILD_VERSION@
 
-daemon_group=@CRM_DAEMON_GROUP@
-daemon_user=@CRM_DAEMON_USER@
-
-daemondir=@CRM_DAEMON_DIR@
-blackboxdir=@CRM_BLACKBOX_DIR@
-configdir=@CRM_CONFIG_DIR@
-penginedir=@PE_STATE_DIR@
-coredir=@CRM_CORE_DIR@
-statedir=@CRM_STATE_DIR@
-schemadir=@CRM_SCHEMA_DIRECTORY@
-ocfdir=@OCF_RA_DIR@
-
 features=@PCMK_FEATURES@
 with_corosync=@SUPPORT_COROSYNC@

[kgaillot]

Your changes look good, though of course we need to rename it to pacemaker-internal :)

I would think we don't actually need a .pc file for this library since it's internal, but looking at pkg-config docs, it seems like we should since we do install the library. Also, maybe it's convenient for some package consistency checkers.

Beyond the scope of this PR, I suspect this file was actually intended for libpe_status and libpe_rules but written wrongly; we're missing configs for those as well as libtransitioner. I'm surprised we don't list our dependencies anywhere, either external (e.g. I suspect pacemaker.pc which is libcrmcommon should list libxml2 in Requires.private) or internal (e.g. I suspect libstonithd should have -lcrmcommon in Libs.private).

[clumens]

Your changes look good, though of course we need to rename it to pacemaker-internal :)

I would think we don't actually need a .pc file for this library since it's internal, but looking at pkg-config docs, it seems like we should since we do install the library. Also, maybe it's convenient for some package consistency checkers.

Beyond the scope of this PR, I suspect this file was actually intended for libpe_status and libpe_rules but written wrongly; we're missing configs for those as well as libtransitioner. I'm surprised we don't list our dependencies anywhere, either external (e.g. I suspect pacemaker.pc which is libcrmcommon should list libxml2 in Requires.private) or internal (e.g. I suspect libstonithd should have -lcrmcommon in Libs.private).

I think we actually need the pkgconfig file for both pacemaker-internal and for libpengine. libtransitioner should have one too, yeah. Every library that we ship is supposed to have one, I think. I can go read about the dependencies stuff and work on a PR for those. I'm not very familiar with them.

[clumens]

Incidentally, I think it would make things a little cleaner if the directory were named lib/internal instead of lib/pacemaker-internal. Check out the EXTRA_DIST, LIBS, and target_LIBS definitions in lib/Makefile.am.

[kgaillot]

Incidentally, I think it would make things a little cleaner if the directory were named lib/internal instead of lib/pacemaker-internal. Check out the EXTRA_DIST, LIBS, and target_LIBS definitions in lib/Makefile.am.

Makes sense

[kgaillot]

Looks good, but some of these files have changed since this was branched, so please rebase on current master (be sure to keep the latest changes)

[clumens]

Yep, I noticed the conflicts and since I don't think I've had to deal with them before on this project, was waiting to see how they should be handled. Coming right up.

[kgaillot]

Awesome :)

I think this is going to help simplify a lot of projects going forward