Created attachment 162885 [details] inotify-tools fix and update When I do (on 10-STABLE) inotifywatch /tmp and then interrupt it (Ctl-c) nothing happens (regardless of intervening events); however, after another event arrives (say, touch /tmp/foo) inotifywatch exits normally and prints the table of events. The same happens when doing, say, inotifywatch -t 30 /tmp. inotifywait works fine, though. The attached patch fixes this (or perhaps works around a libinotify issue) and updates the port to git HEAD since the port uses a more than five years old release. Please see the commit message in the patch for some additional explanations.
Thanks for an update. They didn't make releases for a long time. I will take a look.
Just a clarification: What fixes inotifywatch is not the update to git HEAD (40 is the "patchlevel" from the last release as computed by git --describe tags), but files/patch-src_inotifywatch.c, which is needed because of the way libinotify (currently) works, as my comment in the attached patch tries to convey. I lumped the update and the fix together because git HEAD has, among other bug fixes, one change that I find crucial for even understanding why the code works at all, viz. the change from a polling to a blocking loop in a85bab1.
If they can be isolated, please provide a separate patch with all/any fixes that aren't version updates, so that it can be MFH'd on its own
(In reply to Kubilay Kocak from comment #3) Sorry, I'm not following inotify-tools development, so I'm not familiar enough with its codebase to do this. Also, given that this is a new port anyway, with no other ports depending on it, I don't see how using git HEAD could do any harm here.
(In reply to wjenkner from comment #4) Apologies for not being clearer :) The full change (version update + fix in attachment 162885 [details]) is fine to go into ports HEAD. We also have a ports 'quarterly' branch, which is open for security, bug and other fixed fixes. In particular we prefer not to merge 'version updates' to that quarterly branch. Thus my previous comment was a request to add a separate attachment to this issue, which 'just' includes files/patch-src_inotifywatch.c and/or whatever else is required to do the 'fixes', without updating the version. That way the committer who resolves this issue for you, can commit the 'fix' attachments first, merge it to quarterly, then commit the (version) update
Created attachment 162903 [details] inotifywatch fix for 3.14
Created attachment 162904 [details] Update inotify-tools to git HEAD
(In reply to Kubilay Kocak from comment #5) Thanks, I'm aware of this and I didn't misunderstand you too much, but this doesn't really change my caution that I'm not so familiar with inotify-tools, so that I'm a bit reluctant to cherry-picking changes from upstream. Anyway, the two new patches I attached should do what you asked for, I think, but some independent testing (in particular for the "stable" fix) would be nice.
I already asked the upstream maintainer to make a release. I am going to test on linux to see if this problem exists there too. Then will make the upstream patches and push requests. No need to create additional bug reports IMO. I will take it from here.
(In reply to yuri from comment #9) As I explained in the patch, the problem is due to the way libinotify works. I'm quite sure it does not exist on linux with native inotify.
Hi wjenkner, Thank you for your submission! Could you clarify why this part of the patch is needed? > - event = inotifytools_next_event( 0 ); > + event = inotifytools_next_event( -1 ); the argument is the timeout. Is there the difference between 0 and -1? I submitted two different parts of your patch as two push requests to the upstream. I also sent an e-mail to the upstream maintainer, asking him to accept pull requests and make a new release. Will wait for some time and see if he replies.
(In reply to yuri from comment #11) Please reread this whole PR, in particular koobs@'s comments, my replies and also the commit messages and comments in the patches. The last two patches address koobs@'s plans which he described in detail in this PR. Your course of action does not make sense with respect to these plans. In particular, submitting the patches to upstream at this point is not useful at all. Of course, it is your prerogative as maintainer to have different ideas about this matter but I'd suggest to coordinate them with koobs@.
wjenkner@inode.at, Thanks for your patch and your input. Like I said, I will take it from here. Cheers, Yuri
(In reply to yuri from comment #13) @Yuri Just to be clear, the only request is that the quarterly version can be also be fixed as well. If you intend on a version update (pending a new release upstream) to resolve this issue, please also provide a changeset that addresses only the issue for inotifywatch so it may be merged.
@Kubilay, upstream maintainer replied. He promised to make a release soon. Then the simple version update will solve all issues, since the patches are submitted there, including the fix for this problem with inotifywatch.
(In reply to yuri from comment #15) Except version updates are normally not approved for merging, hence the request for separate changesets. The preferred method/process for this would be to: 1) Commit a bugfix to head NOW, merging it to quarterly 2) Create a new issue for the future version update, commit to head only Unfortunately this issue *also* included a version update, which is why I requested a separate changeset from the reporter, which they have (kindly) provided. I hope this makes more sense, please provide instructions
Created attachment 163214 [details] patch
Created attachment 163215 [details] poudriere log
@Yuri I note there hasn't been a PORTVERSION update in your patch. As a new release hasn't been made yet [1], switching the git revision (and thus code) behind the same PORTVERSION isn't particularly desirable. I'd set a custom DISTVERSION that is higher than 3.14 and highly likely to be less than the next released version number, whenever it lands. What we want is to increment the PORTVERSION, *without* risking a future PORTEPOCH. Also, please clarify regarding the 'fix' patch only for the quarterly branch. [1] https://github.com/rvoicilas/inotify-tools/issues/55
Created attachment 164261 [details] patch Ready to commit. Quarterly is ok.
@Yuri, I'm confused by comment 20. attachment 164261 [details] is ok to merge to quarterly or, is not affected and shouldn't be merged I was under the impression we were aiming for two things: * One patch fixing only inotify-tools (fixing inotifywatch) - for quarterly branch * One patch updating to whatever version you want for head Please clarify and obsolete/create attachments as necessary
This update has two things: * the set of unreleased changes from the upstream that accumulated from the last release few years ago * the patch that wjenkner@inode.at suggested, that is also in the pending pull request upstream There is no reason why upstream won't accept the pull request. So there is no need to make a distinction between these two items. Version update is because this isn't just a cosmetic change. I think it is okay to commit this into both current and quarterly.
A commit references this bug: Author: feld Date: Tue Mar 15 16:58:13 UTC 2016 New revision: 411192 URL: https://svnweb.freebsd.org/changeset/ports/411192 Log: sysutils/inotify-tools: Update to 3.14.01 This update prevents signal handling from being blocked on a worker thread as well as an accumulation of other bug fixes. PR: 204366 MFH: 2016Q1 Changes: head/sysutils/inotify-tools/Makefile head/sysutils/inotify-tools/distinfo head/sysutils/inotify-tools/files/patch-configure.ac head/sysutils/inotify-tools/files/patch-libinotifytools_src_inotifytools.c head/sysutils/inotify-tools/files/patch-src_Makefile.am head/sysutils/inotify-tools/files/patch-src_inotifywatch.c
A commit references this bug: Author: feld Date: Tue Mar 15 17:01:33 UTC 2016 New revision: 411193 URL: https://svnweb.freebsd.org/changeset/ports/411193 Log: MFH: r411192 sysutils/inotify-tools: Update to 3.14.01 This update prevents signal handling from being blocked on a worker thread as well as an accumulation of other bug fixes. PR: 204366 Approved by: ports-secteam (with hat) Changes: _U branches/2016Q1/ branches/2016Q1/sysutils/inotify-tools/Makefile branches/2016Q1/sysutils/inotify-tools/distinfo branches/2016Q1/sysutils/inotify-tools/files/patch-configure.ac branches/2016Q1/sysutils/inotify-tools/files/patch-libinotifytools_src_inotifytools.c branches/2016Q1/sysutils/inotify-tools/files/patch-src_Makefile.am branches/2016Q1/sysutils/inotify-tools/files/patch-src_inotifywatch.c
We normally don't commit bugfixes-only to quarterly, but there seems to be significant enough improvements that it isn't a problem.