devel/libepoll-shim is at version 0.0.20190311, which has a couple of shortcomings that have been fixed upstream[0]. I noticed this when re-plugging a USB keyboard device didn't work, which was due to a problem freeing up kqueue entries in libepoll-shim. I've been told that the fixes in upstream are required to unbreak x11-wm/sway and x11-wm/wayfire (see [1]) The port currently uses a forked repo[2] at github.com/FreeBSDDesktop that has a pending pull request[3] to merge the fixes from upstream. Now, I'm happy to produce a patch to be reviewed by maintainers here.The question is, how it should be done. I would suggest to change the port to use upstream directly and switch to using cmake (and ctest, to run unit tests). If having a cmake dependency is a big problem, we can work around that without forking the repo. If maintainers are unhappy with this, the alternative would be to get the changes merged from upstream into the forked repo at github and subsequently update the port - which would be more work, also on future updates. Given that upstream exists for the purpose of being used on FreeBSD for that exact purpose, I would like to switch to use upstream directly if possible to avoid these complications in the future. If not, some advise on how to get things merged upstream would be nice. [0]https://github.com/jiixyj/epoll-shim/ [1]https://github.com/swaywm/sway/issues/3612 [2]https://github.com/FreeBSDDesktop/epoll-shim [3]https://github.com/FreeBSDDesktop/epoll-shim/pull/5
Can you attach the patch? https://github.com/myfreeweb/freebsd-ports-dank/commit/2bc849661154 can be used as a starting point. How did you test? For example, check the port builds on 11.3 i386, 12.1 amd64, check all consumers build and check a consumer works without rebuild (or bump PORTREVISION).
(In reply to Jan Beich from comment #1) Well, I don't have a patch yet - I simply built upstream locally when I tested. I wanted to know what the maintainers (x11@ in this case I guess - the bug wasn't assigned automatically though) prefer, before spending any time on creating, building, and testing a patch.
FWIW, +1 on this request. Using any Wayland compositor with the older libepoll-shim in the vanilla ports tree is also a major PITA since its implementation of timerfd only allows TIMER_MAX timerfds to be created system wide(!) [1]. End result is that you are limited to <= 32 Wayland clients globally. [1] https://github.com/jiixyj/epoll-shim/issues/8
Created attachment 211137 [details] Update libepoll-shim to upstream repo and latest commit I made this patch to update libepoll-shim to the latest upstream commit. I tested it on 12.0 and 12.1, amd64. I don't have hardware to test it on other versions/archs.
Created attachment 211197 [details] Patch to change libepoll-shim to use actual upstream, test targets, update to 20191117 (In reply to Namkhai B. from comment #4) Thank you for kick-starting things with your patch (y) Please find attached a new version addressing a few issues: - The filename in distinfo was wrong (datestamp was 20191211 instead of 20191117), causing the build to fail in the fetch phase - Once distinfo was fixed, `make check-plist` showed a couple of orphaned files (poudriere is your friend) - Removed RelWithDebInfo, so it builds Release by default and Debug when WITH_DEBUG is set - Added a test target, so we can run `make test` (upstream provides a couple of useful test cases) I tested the patch using `poudriere testport -i` (+ make test) on (11.3|12.1)-RELEASE (amd64|i386). Unit tests 5 and 53 fail on i386, test 5 does so on purpose (it has a comment that it is lacking unit tests for archs != amd64). Test 53 would require some checking (also, this wasn't on native i386, but in a i386 build jail running on amd64). I don't think that this should be a show stopper, as these problems most likely exist in the current version as well (on top of the other issues it has).
Tested on my laptop, I can now start firefox under wayland and xwayland. Also opening multiple terminal under sway.
Comment on attachment 211197 [details] Patch to change libepoll-shim to use actual upstream, test targets, update to 20191117 Compared to Greg's version reviewed by me[1] and linked in comment 1 there're 2 regressions. LICENSE_FILE=${WRKSRC}/LICENSE is required to disambiguate MIT text as there're many styles, see https://fedoraproject.org/wiki/Licensing:MIT USES=compiler:c11 is required to fix build on powerpc* on FreeBSD < 13.0, mips*, riscv64 and sparc64: In file included from test/eventfd-ctx-test.c:13: test/../src/eventfd_ctx.c: In function 'eventfd_ctx_write': test/../src/eventfd_ctx.c:67: warning: implicit declaration of function '__builtin_add_overflow' test/eventfd-ctx-test.c: In function 'tc_threads_read': test/eventfd-ctx-test.c:193: error: incompatible types in assignment test/eventfd-ctx-test.c:221: error: invalid operands to binary == (have 'atomic_int' and 'uint64_t') > +do-test: > + @cd ${TEST_WRKSRC} && ctest -C ${CMAKE_BUILD_TYPE} Define TEST_TARGET=test instead. [1] Happened in a version that became disconnected from lite branch after rebase https://github.com/myfreeweb/freebsd-ports-dank/commit/6b940188d93d
Created attachment 211221 [details] Patch to change libepoll-shim to use actual upstream, test targets, update to 20191117. Also some cleanup based on Jan's feedback (In reply to Jan Beich from comment #7) Thanks for your feedback. I attached another version of the patch that incorporates your findings + cleans up a couple of things portlint found (ordering, COMMENT, and pkg-descr).
Comment on attachment 211221 [details] Patch to change libepoll-shim to use actual upstream, test targets, update to 20191117. Also some cleanup based on Jan's feedback Looks good. > +USES= compiler:c11 cmake Sort USES alphabetically. If unsure use ports-mgmt/portfmt then check the diff. > + > +USE_LDCONFIG= yes > USE_GITHUB= yes Maybe drop newline after USES per bug 231422. However, if you do then a newline before TEST_TARGET is also unnecessary. (In reply to Michael Gmelin from comment #5) > Unit tests 5 and 53 fail on i386, test 5 does so on purpose > (it has a comment that it is lacking unit tests for archs != amd64). > Test 53 would require some checking (also, this wasn't on native > i386, but in a i386 build jail running on amd64). At least running 32-bit Wayland clients on 64-bit compositor works fine. Testing runtime on the oldest supported/alternative architecture (like 11.3 i386 atm) is part of my pre-commit menu e.g., for www/firefox. Some tests seem to fail even on amd64 if run inside poudriere jail.
Created attachment 211223 [details] Patch to change libepoll-shim to use actual upstream, test targets, update to 20191117, more cosmetic changes (In reply to Jan Beich from comment #9) > Sort USES alphabetically. If unsure use ports-mgmt/portfmt then > check the diff. That's what I tried :p. TBH, I hoped that portlint would catch that. Wasn't aware of portfmt, thanks! >> + >> +USE_LDCONFIG= yes >> USE_GITHUB= yes > Maybe drop newline after USES per bug 231422. However, if you do then a > newline before TEST_TARGET is also unnecessary. Ok, why not :) > At least running 32-bit Wayland clients on 64-bit compositor works fine. > Testing runtime on the oldest supported/alternative architecture > (like 11.3 i386 atm) is part of my pre-commit menu e.g., for www/firefox. The first test on i386 fails on purpose, the second one is a sizeof mismatch of some sort (confirmed on i386 on bhyve) - might very well be a problem with the unit test itself (it also fails without a message, which should be fixed upstream - I would leave that to someone else or at least a different point in time though) > Some tests seem to fail even on amd64 if run inside poudriere jail. Which ones though and why/with which error message? I tested about 20 times on 11.3 and 12.1 and had no problems. Two things I noticed: - The unit tests can't run in parallel (e.g. ctest -j4 makes some tests fail). - When testing i386 running on bhyve I've seen a few random failures on timing sensitive tests that didn't happen when running natively/inside a jail.
(In reply to Michael Gmelin from comment #10) > Which ones though and why/with which error message? Add "pre-install: do-test" to Makefile then run "poudriere testport" which. On 11.3 amd64, 12.1 amd64, 13.0 amd64 it consistently shows 30: Test command: /usr/local/bin/cmake "-D" "TEST_FOLDER_NAME=timerfd-test.timerfd__many_timers" "-D" "TEST_EXECUTABLE=/wrkdirs/usr/ports/devel/libepoll-shim/work/.build/test/timerfd-test" "-D" "TEST_NAME=timerfd__many_timers" "-D" "BINARY_DIR=/wrkdirs/usr/ports/devel/libepoll-shim/work/.build/test" "-D" "TIMEOUT=300" "-P" "/wrkdirs/usr/ports/devel/libepoll-shim/work/epoll-shim-1f003fa40f8ac3858dcc474b96cc4aee5820dcd2/test/cmake/ATFRunTest.cmake" 30: Test timeout computed to be: 0 30: -- result: 1, failed: /wrkdirs/usr/ports/devel/libepoll-shim/work/epoll-shim-1f003fa40f8ac3858dcc474b96cc4aee5820dcd2/test/timerfd-test.c:70: (timer_fds[i] = timerfd_create(CLOCK_MONOTONIC, TFD_CLOEXEC | TFD_NONBLOCK)) >= 0 not met 30: CMake Error at /wrkdirs/usr/ports/devel/libepoll-shim/work/epoll-shim-1f003fa40f8ac3858dcc474b96cc4aee5820dcd2/test/cmake/ATFRunTest.cmake:56 (message): 30: 30: /wrkdirs/usr/ports/devel/libepoll-shim/work/epoll-shim-1f003fa40f8ac3858dcc474b96cc4aee5820dcd2/test/timerfd-test.c:70: 30: (timer_fds[i] = timerfd_create(CLOCK_MONOTONIC, TFD_CLOEXEC | 30: TFD_NONBLOCK)) >= 0 not met 30: 30: 30/54 Test #30: timerfd-test.timerfd__many_timers .........................***Failed 0.08 sec 47: Test command: /usr/local/bin/cmake "-D" "TEST_FOLDER_NAME=perf-many-fds.perf_many_fds__perf" "-D" "TEST_EXECUTABLE=/wrkdirs/usr/ports/devel/libepoll-shim/work/.build/test/perf-many-fds" "-D" "TEST_NAME=perf_many_fds__perf" "-D" "BINARY_DIR=/wrkdirs/usr/ports/devel/libepoll-shim/work/.build/test" "-D" "TIMEOUT=15" "-P" "/wrkdirs/usr/ports/devel/libepoll-shim/work/epoll-shim-1f003fa40f8ac3858dcc474b96cc4aee5820dcd2/test/cmake/ATFRunTest.cmake" 47: Test timeout computed to be: 0 47: -- result: 1, failed: /wrkdirs/usr/ports/devel/libepoll-shim/work/epoll-shim-1f003fa40f8ac3858dcc474b96cc4aee5820dcd2/test/perf-many-fds.c:28: eventfds[i] >= 0 not met 47: CMake Error at /wrkdirs/usr/ports/devel/libepoll-shim/work/epoll-shim-1f003fa40f8ac3858dcc474b96cc4aee5820dcd2/test/cmake/ATFRunTest.cmake:56 (message): 47: 47: /wrkdirs/usr/ports/devel/libepoll-shim/work/epoll-shim-1f003fa40f8ac3858dcc474b96cc4aee5820dcd2/test/perf-many-fds.c:28: 47: eventfds[i] >= 0 not met 47: 47: 47/54 Test #47: perf-many-fds.perf_many_fds__perf .........................***Failed 0.02 sec
(In reply to Jan Beich from comment #11) I don't think running unit tests within the poudriere build phase (= contained jail) is helpful, as it has many restrictions. I usually run tests by doing: poudriere testport -j<jailname> -i <category>/<name> ... cd /usr/ports/<category>/<name> make test The reason the tests are failing in this specific case is the default per build jail file descriptor limitation of 1024 open file descriptors imposed by poudriere. To run tests successfully the way you do, you need to raise that limit, e.g. by doing: sysrc -f /usr/local/etc/poudriere.conf MAX_FILES=500000
Created attachment 211521 [details] Patch to change libepoll-shim to use actual upstream, test targets, update to 20191117, even more cosmetic changes In the meantime I opened a pull request upstream to fix unit tests on i386[0]. The new version of the patch includes the changes of that pull request and therefore fixes unit tests on i386. It also changes the do-test target so it skips unit tests that require many fds when run in a restricted environment like poudriere. It does so by checking available file descriptors reported by `ulimit -n'. @jbeich: This will allow you to run unit tests like you described above without altering poudriere.conf. Tested build and unit tests on amd64 and i386, 11.3 and 12.1, with poudriere and without. [0]https://github.com/jiixyj/epoll-shim/pull/14
Comment on attachment 211521 [details] Patch to change libepoll-shim to use actual upstream, test targets, update to 20191117, even more cosmetic changes (In reply to Michael Gmelin from comment #13) > The new version of the patch includes the changes of > that pull request and therefore fixes unit tests > on i386. Thanks! > --- devel/libepoll-shim/files/patch-test_epoll-test.c (nonexistent) > +++ devel/libepoll-shim/files/patch-test_epoll-test.c (working copy) > @@ -0,0 +1,18 @@ > +https://github.com/jiixyj/epoll-shim/commit/0f1665e12f2de53bb49a24415f033a60a4a381d1.diff [...] > --- devel/libepoll-shim/files/patch-test_tst-eventfd.c (nonexistent) > +++ devel/libepoll-shim/files/patch-test_tst-eventfd.c (working copy) > @@ -0,0 +1,14 @@ > +https://github.com/jiixyj/epoll-shim/commit/0f1665e12f2de53bb49a24415f033a60a4a381d1.diff [...] Why not simplify into the following then run "make makesum"? Whoever updates next then only needs to check if your PR was merged. PATCH_SITES= https://github.com/${GH_ACCOUNT}/${GH_PROJECT}/commit/ PATCHFILES+= 0f1665e12f2d.patch:-p1 # https://github.com/jiixyj/epoll-shim/pull/14 > +do-test: > + # Exclude certain tests in resource restricted environments > + @(if [ `ulimit -n` -lt 20100 ]; then \ > + SKIP_TESTS="-E ^(perf-many-fds.perf_many_fds__perf"; \ > + if [ `ulimit -n` -lt 1100 ]; then \ > + SKIP_TESTS=$$SKIP_TESTS"|timerfd-test.timerfd__many_timers"; \ > + fi; \ > + SKIP_TESTS=$$SKIP_TESTS")$$"; \ > + fi; \ Looks ugly but so is checking getrlimit(RLIMIT_NOFILE) in source or USES=cmake:noninja while passing ctest arguments via ARGS variable. I can't suggest anything better. > + cd ${TEST_WRKSRC} && ctest -C ${CMAKE_BUILD_TYPE} $$SKIP_TESTS) Prepend ${TEST_ENV} before ctest command to sanitize HOME.
I'm working on this. I'm looking in to if it's possible to do this update without also switching upstream, as that would give us some benefits, such as using meson instead of cmake (since most other x11@ stuff uses meson or autotools rather than cmake). At today's graphics team meeting we decided that I have until Wednesday evening to work on this, after that manu has approval to commit this.
(In reply to Jan Beich from comment #14) > Why not simplify into the following then run "make makesum"? > Whoever updates next then only needs to check if your PR was merged: I left a comment in the patch. I didn't want to rely on the hash of a commit that might not get merged exactly like it is/end up somehow butchered in github for the build. > Looks ugly but so is checking getrlimit(RLIMIT_NOFILE) in source I spent like an hour trying to find a solution that looks nice/readable... (all the other options were worse) ¯\_(ツ)_/¯ > Prepend ${TEST_ENV} before ctest command to sanitize HOME. Like in "${SETENV} ${TEST_ENV} ctest..."?
(In reply to Niclas Zeising from comment #15) Thank you for working on this. I think avoiding a build system by forking is not such a great idea, especially since cmake is used by so many ports and one always ends up installing it anyway[0]. If you're really dedicated to keep cmake out, I would suggest to: a) Document this in the porters handbook ("areas of the ports tree that avoid cmake", e.g. dependencies of xorg-server) b) Do the changes necessary for this to happen as patches in the port skeleton and still switch to the original upstream. Then retire our fork. This will remove one indirection that makes it unnecessarily painful and complicated to contribute. c) Make sure `make test` still runs the unit tests The fork has to be maintained. Assuming that epoll-shim isn't abandoned, future changes need to be merged into our fork. If our fork uses a different build system, that's extra work on every update that has to happen before the port can be updated; in a different place, involving different people or at least different communication channels and processes. So this isn't just about this one update, but about every future update down the road. [0] make -C /usr/ports search bdeps=cmake display=name
(In reply to Michael Gmelin from comment #16) > I left a comment in the patch. I didn't want to rely on the hash > of a commit that might not get merged exactly like it is/end up > somehow butchered in github for the build. GitHub allows referencing commits disconnected from all branches. I use it all the time in my ports to pull random patches: upstreamed or not, even from deleted repos. Besides, your comment also refers to a commit "that might not get merged exactly". Nevermind then. How to format patches varies by maintainer. > Like in "${SETENV} ${TEST_ENV} ctest..."? Yep. I suspect ${SETENV} is more declarative than functional. Maybe in the past it allowed to use C shell to run commands from a target but nowadays a lot of ports rely on Bourne syntax with some Almquist/FreeBSD additions. Porter's Handbook doesn't cover ${SETENV} while portlint doesn't warn if ${CONFIGURE_ENV}, ${MAKE_ENV}, ${TEST_ENV}, etc. are used without ${SETENV}.
Created attachment 211545 [details] Patch to change libepoll-shim to use actual upstream, test targets, update to 20191117, even *more* cosmetic changes (In reply to Jan Beich from comment #18) > GitHub allows referencing commits disconnected from all > branches. I use it all the time in my ports to pull random > patches: upstreamed or not, even from deleted repos. I'm more concerned about me force pushing by accident :D I will keep that neat trick in mind though and certainly will apply in in the future - thanks! > Besides, your comment also refers to a commit "that might > not get merged exactly". That's a good point, I changed the comments to point to the pull request instead. > Yep. I suspect ${SETENV} is more declarative than functional. Seems like most examples I found in the tree use it that way - so you're right that it's more of declarative nature than anything. It could also help in case something really stupid happens to $TEST_ENV, even though in practice that's probably wishful thinking.
Created attachment 211561 [details] Patch to change libepoll-shim to use actual upstream, test targets, update to 20200211, even *more* cosmetic changes Patch to update to 20200211, the only difference to the previous patch is that upstream merged the pull request to fix unit tests on i386.
Created attachment 211562 [details] Patch to change libepoll-shim to use actual upstream, test targets, update to 20200211, final cosmetic changes This should be the final patch, adding output to inform the user in case unit tests are skipped.
A commit references this bug: Author: zeising Date: Wed Feb 12 22:59:05 UTC 2020 New revision: 525983 URL: https://svnweb.freebsd.org/changeset/ports/525983 Log: devel/libepoll-shim: Update to latest snapshot Update the snapshot of devel/libepoll-shim This solves several issues when using wayland PR: 243649 (with minor changes) Submitted by: grembo Changes: head/devel/libepoll-shim/Makefile head/devel/libepoll-shim/distinfo head/devel/libepoll-shim/pkg-descr head/devel/libepoll-shim/pkg-plist
Committed, leave open for a day or two just in case.
(In reply to commit-hook from comment #22) Just for the records, "with minor changes" refers to not changing upstream and keep using the FreeBSDDesktop fork on github instead. At this moment the fork is basically identical to upstream (besides CI integration). @zeising Not sure if you saw my comments above[0], I think that keeping a fork of something that has native FreeBSD support and is actively maintained by a responsive author creates extra work and complicates contributing. If you want to keep it like that, I'd suggest you close the issue and pull request which were created by the upstream author: https://github.com/FreeBSDDesktop/epoll-shim/pull/5 https://github.com/FreeBSDDesktop/epoll-shim/issues/4 Maybe also add some comments in your fork's README.md on github, explaining how people are supposed to contribute if they find problems: - Pull request on your fork on github - Pull request upstream - Bug here - All three of the above(?) Thanks [0]https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=243649#c17
I suggested to Niclas that we should contact the author and see if he will be ok to move the repo under github.com/FreeBSD This make no sense to have this repo under FreeBSDDesktop and this will avoid a new fork if the original author goes MIA.
(In reply to Emmanuel Vadot from comment #25) Thank you for your response. At the moment the author is actively maintaining the software and responds to pull requests and open issues in a timely manner. He was even kind enough to open pull requests against our fork. What I would do is ask him to introduce tagged versions, so portscout can inform us that it's time to update the port. So, unless there is a history of animosities with that specific author, forking an actively maintained project to prepare for him going MIA in the future seems quite unorthodox to me. All it does is create extra work for us and slow down progress. I would only fork once upstream becomes stale or uncooperative. This doesn't seem to be the case here. Assuming there will be an actual need to fork epoll-shim in the future, it might make more sense to do it under github.com/freebsd though, as its usefulness may not be limited to the desktop and having epoll compatibility could be useful when porting all kinds of software from linux.
(In reply to Michael Gmelin from comment #26) The FreeBSD port of libepoll-shim has always used the one in FreeBSDDesktop GitHub. When it was imported into the FreeBSD ports tree that was the actively maintained version, and it still is. There are some outstanding issues, such as taking care of pull requests (that are mostly merged) and so on that I didn't have time to do last night, I wanted to prioritize actually getting this out the door.
(In reply to Niclas Zeising from comment #27) There's no point in fighting over this. If you consider the version at FreeBSDDesktop to be upstream and maintained you might want to change WWW: in pkg-descr to point to that version, so that issues and pull requests are opened there and not on jiixyj's repo. Speaking of contributing, is there any documentation you could point me to that explains how ports maintained by x11@ are managed and how that relates to the graphics team and the FreeBSDDesktop account? I already saw deck from FOSDEM 2019, cool stuff.
I believe this can be closed.