Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#52144 closed enhancement (fixed)

mariadb: support for mpkg / mdmg

Reported by: ctreleaven (Craig Treleaven) Owned by: pixilla (Bradley Giesbrecht)
Priority: Normal Milestone:
Component: ports Version:
Keywords: Cc: mkae (Marko Käning)
Port: mariadb-server

Description

My mythtv ports rely on mariadb as the backend database. This ticket proposes some enhancements to mariadb to better support packaging via an mpkg or mdmg.

First, mariadb (5.5) still uses an antiquated startupitem which in turn relies on daemondo. I believe the new launchd (10.10 and later?) no longer supports startupitems. In any event, it is a problem to include daemondo in a pkg. Therefore, I've adapted the launchd plist introduced in MySQL 5.7.8 to work with mariadb in the MacPorts environment. Patch attached for review and comments.

Second, I'm developing packaging scripts (preinstall, postflight, etc) such that manual intervention will not be required after an mpkg / mdmg installation. The database will be functional and running. This is different than the state after 'sudo port install mariadb-server' but I think it makes sense. Most users installing from an mpkg don't expect to have to do manual steps afterwards to have functional software. A patch is in process for this and will hopefully be submitted in a few days.

Attachments (6)

patch_port_mariadb-server_startupitem_2016Aug31.diff (3.2 KB) - added by ctreleaven (Craig Treleaven) 8 years ago.
patch_port_mariadb-server_startupitem_and_pkg_2016Sep04.diff (6.9 KB) - added by ctreleaven (Craig Treleaven) 8 years ago.
patch_port_mariadb-server_startupitem_and_pkg_2016Sep06.diff (7.1 KB) - added by ctreleaven (Craig Treleaven) 8 years ago.
Installer Log 16-Jan-2017_1.txt (25.7 KB) - added by ctreleaven (Craig Treleaven) 7 years ago.
patch_port_mariadb-server_support_packaging.diff (3.8 KB) - added by ctreleaven (Craig Treleaven) 7 years ago.
complete patch (git tried to trip me up!)
0001-mariadb-server-support-mpkg-mdmg-packaging-with-scri.patch (4.2 KB) - added by ctreleaven (Craig Treleaven) 7 years ago.

Download all attachments as: .zip

Change History (28)

Changed 8 years ago by ctreleaven (Craig Treleaven)

comment:1 Changed 8 years ago by ctreleaven (Craig Treleaven)

BTW, the MySQL launchd plist that I started with [1] contains some unfamiliar keys:

+    <key>ProcessType</key>       <string>Interactive</string>
+    <key>SessionCreate</key>     <true/>

I don't have an easy way to test this with older Mac OS X versions (pre 10.9). Could someone confirm whether they cause a problem in such an environment?

I believe neither key is strictly necessary for our mariadb-server implementation and they could easily be removed if they present a problem.

[1] https://docs.oracle.com/cd/E17952_01/mysql-5.6-en/osx-installation-launchd.html

Last edited 7 years ago by ryandesign (Ryan Carsten Schmidt) (previous) (diff)

comment:2 Changed 8 years ago by mf2k (Frank Schima)

Owner: changed from pixilla to pixilla@…
Version: 2.3.4

Trac requires complete email addresses.

comment:3 Changed 8 years ago by ctreleaven (Craig Treleaven)

Attached is a new patch that adds packaging support as well as a couple of small tweaks to my previous patch. At over 150 lines, I know it is a lot to take in but it breaks down into logical groups. As in the previous patch, there are sections to copy the launchd plist out of ${filespath} and update the ${prefix} and port name. Then there are post-activate and pre-deactivate blocks to link/unlink the plist into /Library/LaunchDaemons. Then 29 lines for the plist itself.

Now added is a pre-pkg block. This copies the two scripts (preinstall and postinstall) from ${filespath} and fixes the @NAME@ and @PREFIX@ placeholders. Also, ensures that the scripts have 0755 permissions. The preinstall script uses 'launchctl list' to check if the deamon is supposed to be running. If so, it unloads it. The postinstall script:

  • forces the launchd plist to 0644 permissions. I don't think the _should_ be necessary but sometimes had problems during testing. Easier just to use a sledgehammer and force it.
  • remove the old link in /Library/LaunchDaemons (if one is there) and put in the new one
  • force the right user/group on our directories in var/run, var/db and var/log.
  • run the upstream database initialization script, mysql_install_db.
  • again checks for a loaded daemon and unloads it if it is. Again ought to be unnecessary but harmless if the preinstall script did its job.
  • finally, use launchctl to load the daemon. The command line arguments in the launchd plist should bring up a minimally configured db server. Subsequently, the user (or another installer) can add a my.cnf configuration file to customize as desired.

In addition, I made a gratuitous change to the long_description. (Wording is a mash-up from the MariaDB About page.) This description appears on the Installer splash screen after packaging and seems a little more descriptive of what the user is about to inflict on their system.

Barring objections, I plan commit these changes under openmaintainer in the next few days. I feel the startupitem to launchd plist changes are relatively low risk. I'll try to handle anything that might crop up.

My packaging-related changes only have any effect if one is creating an mpkg or mdmg. My impression is that there are very few people doing that--it would not work now because of the daemondo problem.

Changed 8 years ago by ctreleaven (Craig Treleaven)

comment:4 Changed 8 years ago by ctreleaven (Craig Treleaven)

Revised patch attached. Now respects the setting of startupitem.install.

I confirm that the 'sudo port load | unload' function as expected.

Changed 8 years ago by ctreleaven (Craig Treleaven)

comment:5 Changed 8 years ago by mf2k (Frank Schima)

Note that revision_server should be 1 (not 4) in your patch.

comment:6 Changed 8 years ago by ctreleaven (Craig Treleaven)

Revisions don't _have_ to go sequentially, do they? I'm using the intermediate revisions internally for A-B comparison.

Unfortunately, I have a new patch. Even though the installer was reporting success, part of one script was failing. The installer runs the pre|postinstall scripts inside a sandbox that does not have access to /Private, among other things. Thus, no access to /tmp. This caused mysql_install_db to stumble. Fixed that by telling it to use the datadir for temp files.

Sorry that I didn't catch this earlier.

comment:7 Changed 8 years ago by pixilla (Bradley Giesbrecht)

As discussed in direct email with ctreleaven these patches need more testing before committing. I should have a commit ready later this week or next.

comment:8 Changed 7 years ago by pixilla (Bradley Giesbrecht)

Partially resolved in r153724.

comment:9 Changed 7 years ago by ctreleaven (Craig Treleaven)

Updated patch for the pre-pkg phase. Prepares preinstall and postinstall scripts and creates user-friendly description for the installer splash screen. (Could also supply as a pull request if someone can walk me though the process.)

Installer log from test installation into a VM also attached.

Changed 7 years ago by ctreleaven (Craig Treleaven)

Changed 7 years ago by ctreleaven (Craig Treleaven)

complete patch (git tried to trip me up!)

comment:10 Changed 7 years ago by pixilla (Bradley Giesbrecht)

In attachment:patch_port_mariadb-server_support_packaging.diff files/postinstall:13 removes a file belonging to a port and later replaces it with a different file. I think this should be avoided.

Would it work to add "pkg" to the launchd plist for the packaged version?

ln -s @PREFIX@/etc/LaunchDaemons/org.macports.@SUBPORT@/org.macports.@SUBPORT@.plist \
  /Library/LaunchDaemons/org.macports.@SUBPORT@.pkg.plist || exit 1

And change files/postinstall:37 to:

/bin/launchctl load -w /Library/LaunchDaemons/org.macports.@SUBPORT@.pkg.plist

comment:11 in reply to:  10 Changed 7 years ago by ctreleaven (Craig Treleaven)

Replying to pixilla:

In attachment:patch_port_mariadb-server_support_packaging.diff files/postinstall:13 removes a file belonging to a port and later replaces it with a different file. I think this should be avoided.

This one of the files where ownership/permissions sometimes gets mangled during the install. Deleting and recreating the link seemed like the most direct way of avoiding that potential problem.

Would it work to add "pkg" to the launchd plist for the packaged version?

ln -s @PREFIX@/etc/LaunchDaemons/org.macports.@SUBPORT@/org.macports.@SUBPORT@.plist \
  /Library/LaunchDaemons/org.macports.@SUBPORT@.pkg.plist || exit 1

And change files/postinstall:37 to:

/bin/launchctl load -w /Library/LaunchDaemons/org.macports.@SUBPORT@.pkg.plist

That would work.

comment:12 Changed 7 years ago by mkae (Marko Käning)

Cc: mkae added

comment:13 Changed 7 years ago by ctreleaven (Craig Treleaven)

Cc: mkae removed

New patch attached using Ryan's tip about using xinstall.

Re the launchd plist name, adding 'pkg' isn't appropriate. Keep in mind that a standalone installer only makes sense where MacPorts isn't installed. We won't be changing anything owned by port because there are no ports!

The postinstall script may be run on a virgin system or where a previous version of our payload is already in place. Say the user had used our installer for MariaDB 5.5.n and now has our new installer for MariaDB 5.5.n+1. In this case, we need to have launchd unload the old version before we mess with it. With a 'belt and suspenders' approach, I attempt the unload both in the preinstall script and the postinstall script. After our n+1 payload is copied into place, the postinstall script uses launchd to load the new version so the user can go merrily along their way.

Does that clarify?

Changed 7 years ago by ctreleaven (Craig Treleaven)

comment:14 Changed 7 years ago by mkae (Marko Käning)

Cc: mkae added

comment:15 Changed 7 years ago by ctreleaven (Craig Treleaven)

May I push?

comment:16 Changed 7 years ago by ctreleaven (Craig Treleaven)

Resolution: fixed
Status: newclosed

comment:17 in reply to:  13 Changed 7 years ago by pixilla (Bradley Giesbrecht)

Replying to ctreleaven:

New patch attached using Ryan's tip about using xinstall.

Re the launchd plist name, adding 'pkg' isn't appropriate. Keep in mind that a standalone installer only makes sense where MacPorts isn't installed. We won't be changing anything owned by port because there are no ports!

The postinstall script may be run on a virgin system or where a previous version of our payload is already in place. Say the user had used our installer for MariaDB 5.5.n and now has our new installer for MariaDB 5.5.n+1. In this case, we need to have launchd unload the old version before we mess with it. With a 'belt and suspenders' approach, I attempt the unload both in the preinstall script and the postinstall script. After our n+1 payload is copied into place, the postinstall script uses launchd to load the new version so the user can go merrily along their way.

Does that clarify?

We cannot control what others do. You assume no one will ever use "pkg" on a system where port is installed. This feels like it is designed to break and I would like this change reverted.

comment:18 Changed 7 years ago by ctreleaven (Craig Treleaven)

See https://guide.macports.org/#using.binaries.binary-packages -- there is a prominent warning not to create installers that write into /opt/local. In other words, don't mix the two delivery systems. There is a similar warning in man port-mpkg.

Using a pkg to install into a prefix owned by MacPorts is a very good way to break things in general. Each component of an installer package is at a particular version. Generally they will be the current version at the time the installer is cut but it doesn't take very long before an installer would be overwriting newer stuff with older. There is no version checking or any way to check for incompatibilities.

Also, note that I've added packaging support to the fontconfig and shared-mime-info ports in the last few weeks. There was no objection to those commits.

MacPorts provides tools to do packaging. They can be misused but this commit was about making MariaDB work out-of-the-box when it is included in an mpkg. I think the commit is useful and poses minimal extra risk (if any).

comment:19 in reply to:  18 Changed 7 years ago by pixilla (Bradley Giesbrecht)

Replying to ctreleaven:

See https://guide.macports.org/#using.binaries.binary-packages -- there is a prominent warning not to create installers that write into /opt/local. In other words, don't mix the two delivery systems. There is a similar warning in man port-mpkg.

Regardless of the prefix, your changes rm /Library/LaunchDaemons/org.macports.mariadb-server.plist and is therefore incompatible with the MacPorts mariadb-server port.

Is there a problem with adding "pkg" to the name of your packages launchd plist to remove the file name collision?

Last edited 7 years ago by pixilla (Bradley Giesbrecht) (previous) (diff)

comment:20 Changed 7 years ago by ctreleaven (Craig Treleaven)

I understand your concern about conflicts but I maintain that users should not mix MacPorts and package installers. Even if the installer uses a custom prefix (I use /opt/dvr), as you rightly point out, there can be collisions in /Library/LaunchDaemons and also /Library/LaunchAgents and /Applications/MacPorts. It might be better if installers created with MacPorts could look for an existing MacPorts installation (the presence of the port command, perhaps) and alert the user to the potential for conflicts. Going further, the user might also have installed MySQL from their dmg (1) and now the various servers may be fighting over the network port. At some point, the user has to take responsibility for what they're installing on their system.

An easy improvement would be to expand the official MacPorts documentation to be more explicit about the issues and limitations of using MacPorts to produce installers. I've been thinking about adding a wiki page with tips that I've gleaned.

Right now, we strongly discourage folks from using MacPorts and any other package manager because of the potential for conflicts. The port diagnose command checks for such issues but I don't see any way for it to identify if the user has also used other MacPorts-produced installers.

My MythTV.28 installer includes 4 daemons: mythbackend, mariadb, apache2 and logrotate. Three of the packages add stuff to /Applications/MacPorts (Myth, Python 2.7 and Qt 5). I've been producing these installers for 3 years and I've only had one case where a user wanted to switch to MacPorts. My docs have a big warning about choosing one approach or the other.

AFAICT, I'm one of the very few people using MacPorts to produce an installer (2). The installer for Myth 0.27 has been downloaded over 12,000 times. I agree that there is *potential* for collisions. It just doesn't seem to be a big problem in actual practise.

(1) https://dev.mysql.com/downloads/file/?id=467574

(2) https://sourceforge.net/projects/macportsmythtvinstaller/

comment:21 in reply to:  20 ; Changed 7 years ago by pixilla (Bradley Giesbrecht)

If there is no problem with adding "pkg" to the name of your packages launchd plist to remove the file name collision please do so or explain the problem this solution presents.

comment:22 in reply to:  21 Changed 7 years ago by ctreleaven (Craig Treleaven)

Replying to pixilla:

If there is no problem with adding "pkg" to the name of your packages launchd plist to remove the file name collision please do so or explain the problem this solution presents.

The main problem is that my installer will be used to upgrade a lot of existing MythTV 0.27 systems and the existing launchd plist is already named org.macports.mariadb-server.plist.

Plus, as I tried to explain above, I don't think this will pose a significant problem going forward. Even if somebody does mix MacPorts and then an installer containing MariaDB, all they have to do to get back to normal is:

sudo port unload mariadb
sudo port deactivate mariadb # plist is deleted
sudo port activate mariadb # new plist points to MacPorts-installed version
sudo port load mariadb

Seems like a simple-enough workaround should it be needed.

Note: See TracTickets for help on using tickets.