Page MenuHomeFreeBSD

databases/automysqlbackup: add initial version of port (3.0_rc6)
ClosedPublic

Authored by me_cschwarz.com on Mar 17 2019, 1:48 PM.

Details

Summary

port databases/autobackupmysql exists, but does not have the same featureset

Test Plan

Works in my MySQL jail (mariadb-103-{server,client})

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

matthew added inline comments.
databases/automysqlbackup/Makefile
28 ↗(On Diff #55155)

${MKDIR} already includes the '-p' flag: no need to repeat it.

databases/automysqlbackup/pkg-plist
3 ↗(On Diff #55155)

You aren't recreating the 0700 directory permissions that you set in the makefile -- this will end up wit 0755 permissionns if you install from pkg.

address first review round remarks
(where are the docs for @dir params?)

databases/automysqlbackup/pkg-plist
3 ↗(On Diff #55155)

where is the documentation for @dir?

mmokhi added a subscriber: mmokhi.Mar 24 2019, 11:29 AM
mmokhi added inline comments.
databases/automysqlbackup/Makefile
29 ↗(On Diff #55192)

I'm not sure if I understood @matthew 's feedback on this line...
But I would personally let the chmod stuff being done in pkg-plist instead of Makefile... (Or at least either all here or all there)...

databases/automysqlbackup/files/patch-automysqlbackup
9 ↗(On Diff #55192)

Maybe it's too much what I'm pointing, but I often use %LOCALBASE% (or in this case %ETCDIR%) and the ${REINPLACE_CMD} in post-patch 😅
Not saying it's better or worse to do so, but might help other people (who have diff place that /usr/local to use it as well 😊

databases/automysqlbackup/pkg-plist
3 ↗(On Diff #55155)

This is probably what you are looking for. 👍

matthew added inline comments.Mar 24 2019, 12:04 PM
databases/automysqlbackup/Makefile
29 ↗(On Diff #55192)

When you are building a port, all the Makefile installation commands are doing is copying files into ${STAGEDIR}. The contents of ${STAGEDIR} are then turned into a pkg -- either gathered into a pkg tarball, or in it's installed form with files spread across your active filesytem. This happens whether or not you install directly from your ports tree, or whether you create a pkg tarball, stick it into a repository and download and install from there.

You're meant to be able to install to ${STAGEDIR} and thence build a pkg as an unprivileged user, which means that file ownership and permissions in ${STAGEDIR} can't just be copied into the pkg generated from ${STAGEDIR}, and thence into the main filesystem when the pkg is installed -- in fact, they are ignored, other than copying over any execute permissions. In particular anything like setuid or setgid bits are stripped; regular files default to root:wheel and mode 0644 or 0755 if executable, and directories to root:wheel and mode 0755. If you need any other permissions or ownership, this needs to be set in the pkg-plist.

In summary: permissions and ownership in ${STAGEDIR} are (almost) irrelevant: it's what's set in pkg-plist that counts.

mmokhi added inline comments.Mar 24 2019, 12:09 PM
databases/automysqlbackup/Makefile
29 ↗(On Diff #55192)

Aha, yes.
Makes sense, I believe that's why we basically have the permission and ownership stuff in pkg-plist (as I suggested; more explanation available here).

Thanks for complete and reasonable explanation 🙏 👍

databases/automysqlbackup/Makefile
29 ↗(On Diff #55192)

Any action required on my part? Otherwise please mark this as done.

databases/automysqlbackup/files/patch-automysqlbackup
9 ↗(On Diff #55192)

When is %ETCDIR% replaced then? This is a patch file... Does the ports tree use some magic version of patch(1)?

mmokhi added inline comments.Mar 26 2019, 11:32 AM
databases/automysqlbackup/Makefile
29 ↗(On Diff #55192)

I guess, It makes sense to update your patch deleting the chmod stuff from Makefile and handle all the rest in pkg-plist 😊
Then the rest is fine IMHO 👍

databases/automysqlbackup/files/patch-automysqlbackup
9 ↗(On Diff #55192)

${REINPLACE_CMD} in post-patch target 😊
letting the patch file put the %ETCDIR% then you sed the patched file

me_cschwarz.com marked an inline comment as done.
  • remove chmod from post-install (covered by pkg-plist)
me_cschwarz.com marked an inline comment as done.Mar 27 2019, 10:25 AM
mmokhi accepted this revision.Mar 27 2019, 10:32 AM

The %ETCDIR% thingie I commented about might be as well good to take care of
(If people with paths different that /usr/local are using your port for example).
But I don't think it's a necessity at the moment 🙂
You can take care of it later 👍

Macro lgtm:

Macro shipit:

This revision is now accepted and ready to land.Mar 27 2019, 10:32 AM

Just replacing /usr/local/etc with %ETCDIR% doesn't work on my machine.
Is it because I override do-install?

Just replacing /usr/local/etc with %ETCDIR% doesn't work on my machine.
Is it because I override do-install?

Sorry, maybe my comment was a bit misleading.
After replacing stuff with %ETCDIR% or anything in the patch-files, of course you need a post-patch: target with
a ${REINPLACE_CMD} in it to replace the %ETCDIR% value with ${ETCDIR}.

This revision now requires review to proceed.Mar 27 2019, 11:13 AM
mmokhi accepted this revision.Mar 27 2019, 11:16 AM

Thanks.
To me it's perfect now 😊

Macro lgtm:

Macro everything-is-fine:

This revision is now accepted and ready to land.Mar 27 2019, 11:16 AM
This revision was automatically updated to reflect the committed changes.