Hi Marc, Thank you for taking the time to review this package. For the purposes of the RFS I'm requesting commit: 38ced3f on the experimental branch. I've deleted all old uploads on mentors to remove any ambiguity.
On Tue, Sep 26, 2017 at 04:54:13PM +0200, Marc Haber wrote: > On Sat, Sep 23, 2017 at 06:45:24PM -0400, Nicholas D Steeves wrote: > > Sorry for the non-functional upstream sources when building with gbp. > > I just noticed that today and have fixed it. I've pushed to the > > experimental branch of the git repo. I've also enabled the systemd > > timer by default in experimental. > > Is it intended that the daemon doesn't get reloaded on package > installation and that btrfsmaintenance-refresh needs to be started > manually? For the packaging on the master branch, yes. For the packaging on the experimental branch the service is started after the package is installed. > Some short comments. [...] > - if the scripts are intended to be called manually as well, they should > live in /usr/[s]bin, and not in /usr/share I guess it would be possible to maintain a patch for each of *.sh that: - . $(dirname $(realpath $0))/btrfsmaintenance-functions + . /usr/share/btrfsmaintenance/btrfsmaintenance-functions That said, I believe that the primary purpose of the scripts is automation and that they are not intended to be called manually. There is currently a pull request ( https://github.com/kdave/btrfsmaintenance/pull/36 ) for enhanced systemd support. After that, or something similar is merged I believe that the recommended way to manually trigger something will be as a oneshot systemd maintenance job. I think the concept behind btrfsmaintenance is as follows: Configure it once, then forget about btrfs-related maintenance tasks. > - Can I tell the scripts "do your operation on _this_ btrfs file system, > and do it now", or do they only operate on file systems that are in > /etc/fstab? At this time, btrfsmaintenance is not general-purpose convenience wrapper around the btrfs balance, defrag, and scrub commands. Each of the scripts gets its configuration from /etc/default/btrfsmaintenance and does not take any arguments. From evaluate_auto_mountpoint() in btrfsmaintenance-functions: # this function checks whether the variable contains the special keyword "auto" # if yes, all currently mounted btrfs filesystems are evaluated and their mountpoints # are put into the parameter variable "auto" uses a findmnt, then 'find -xdev' per mount point. I ought to open an upstream issue to allow blacklisting of removable devices. > - manpages are missing Is a manpage appropriate for something that is evolving into a set of script-backed systemd services? If they were stand-alone wrapper/convenience scripts that accepted arguments of arbitrary paths then I would definitely agree. As it stands I imagine that a manpage would only cover the fact that configuration is in /etc/default/btrfsmaintenance, plus links/stubs to/for btrfs-scrub(8), btrfs-filesystem(8)--for defrag--and btrfs-balance(8). Whether as a manpage, a README.Debian, comments in /etc/default/btrfsmaintenance, or the upstream README, what can I do to make this better? Honestly, I'm sympathetic to upstream's position, because systemd units such as alsa-restore do not have (and imho do not need) manpages. Upstream conversation on this topic is here: https://github.com/kdave/btrfsmaintenance/issues/4 Kind regards, Nicholas
signature.asc
Description: PGP signature
