|
Description
Simon McVittie
2015-12-01 17:59:43 UTC
Created attachment 120226 [details] [review] Avoid -Wunused-label when compiling with libselinux but no libaudit Created attachment 120227 [details] [review] [2/10] cmake: manual-tcp is not an automated test Don't run it when we run automated tests. Created attachment 120228 [details] [review] [3/10] cmake: copy the systemd-activation directory too It is needed for a couple of test-cases. Created attachment 120229 [details] [review] embedded tests: accept and ignore --tap argument This makes them semi-command-line-compatible with a way we can invoke the GLib-based tests to get more useful debug logs. These tests still do not actually produce TAP output yet; I tried implementing that, but it requires changing a lot of noise on stdout to come out of stderr, and there was something weird going on with subprocesses restarting the test numbering which will need further investigation before making that change. Created attachment 120230 [details] [review] [05/10] cmake: run all automated tests with --tap for better diagnostics Created attachment 120231 [details] [review] [06/10] _dbus_test_oom_handling: allow disabling it as documented We document DBUS_TEST_MALLOC_FAILURES=0 in HACKING, but it didn't actually work: we'd iterate from i=-1 to i=0. Created attachment 120232 [details] [review] [07/10] Do not require systemd to have a service file if using it for activation With --systemd-activation we special-case the name org.freedesktop.systemd1 by assuming that it will eventually connect to the bus. With that in mind, we can ignore whether it has a .service file, and let it be "activated" regardless. This fixes a regression test failure on non-systemd systems such as the Ubuntu 14.04 OS on travis-ci.org: UpdateActivationEnvironment failed, because it tried to update the (fake) systemd environment, but because systemd was not actually installed, there was no service file for it in the system's search paths. We could address this by placing a dummy service file with Exec=/bin/false in our search path like the real systemd does, but it seems cleaner to not require this; this would eventually enable the real systemd to stop installing that dummy service file. This would not happen outside the regression tests, because there is no sense in using --systemd-activation without systemd installed. Created attachment 120233 [details] [review] [08/10] Fix shell syntax for installcheck-local with no DESTDIR A closing brace must be preceded by a semicolon. The CI integration added later in this branch actually runs "make installcheck" with no DESTDIR; apparently nobody else has ever tried that. Created attachment 120234 [details] [review] [09/10] Don't try to run manual tests in installcheck We were mistakenly running all installed executables, even manual tests that never terminate. Created attachment 120235 [details] [review] [10/10] add travis-ci.org build machinery The idea is that .travis.yml is specific to Travis-CI, but most of the actual work is done in tools/ci-build.sh, which should be reasonably CI-platform-agnostic (it currently assumes that build-dependendencies are preinstalled, that the "native" platform we're building on is GNU/Linux or something very close, and that "mingw" means mingw-w64 as packaged in Debian and Ubuntu). Comment on attachment 120226 [details] [review] Avoid -Wunused-label when compiling with libselinux but no libaudit Review of attachment 120226 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 120227 [details] [review] [2/10] cmake: manual-tcp is not an automated test Review of attachment 120227 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 120228 [details] [review] [3/10] cmake: copy the systemd-activation directory too Review of attachment 120228 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 120230 [details] [review] [05/10] cmake: run all automated tests with --tap for better diagnostics Review of attachment 120230 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 120233 [details] [review] [08/10] Fix shell syntax for installcheck-local with no DESTDIR Review of attachment 120233 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 120234 [details] [review] [09/10] Don't try to run manual tests in installcheck Review of attachment 120234 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 120229 [details] [review] embedded tests: accept and ignore --tap argument Review of attachment 120229 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 120235 [details] [review] [10/10] add travis-ci.org build machinery Review of attachment 120235 [details] [review]: ----------------------------------------------------------------- Looks good Note: x86_64 mingw target is missing Question: How are these builds triggered, on git HEAD commit hash change ? ::: .travis.yml @@ +43,5 @@ > + - dbus_ci_buildsys=cmake > + - dbus_ci_host=mingw > + - dbus_ci_host=mingw dbus_ci_buildsys=cmake > + > +# vim:set sw=2 sts=2 et: I do not know exactly this syntax, but it looks reasonally. ::: cmake/i686-w64-mingw32.cmake @@ +16,5 @@ > +set(CMAKE_RC_COMPILER ${COMPILER_PREFIX}windres) > + > +set(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM NEVER) > +set(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY ONLY) > +set(CMAKE_FIND_ROOT_PATH_MODE_INCLUDE ONLY) Looks like a minimal setting, obs uses more variables (search %_mingw32_cmake at https://build.opensuse.org/package/view_file/windows:mingw:win32/mingw32-filesystem/macros.mingw32?expand=1) ::: tools/ci-build.sh @@ +20,5 @@ > +fi > + > +dbus_test=yes > +dbus_test_fatal=yes > + if test "$dbus_ci_buildsys" == "autotools"; then @@ +21,5 @@ > + > +dbus_test=yes > +dbus_test_fatal=yes > + > +NOCONFIGURE=1 ./autogen.sh fi @@ +127,5 @@ > + set "$@" LDFLAGS=-L"${mingw}/lib" > + set "$@" CPPFLAGS=-I"${mingw}/include" > + set "$@" CFLAGS=-static-libgcc > + set "$@" CXXFLAGS=-static-libgcc > + # don't run tests yet, Wine needs Xvfb and You also need a wine prefix. on obs I use this https://build.opensuse.org/package/show/home:rhabacker:branches:windows:mingw:win32/mingw32-cross-wine @@ +182,5 @@ > + set "$@" -D CMAKE_LIBRARY_PATH="${mingw}/lib" > + set "$@" -D EXPAT_LIBRARY="${mingw}/lib/libexpat.dll.a" > + set "$@" -D GLIB2_LIBRARIES="${mingw}/lib/libglib-2.0.dll.a" > + set "$@" -D GOBJECT_LIBRARIES="${mingw}/lib/libgobject-2.0.dll.a" > + set "$@" -D GIO_LIBRARIES="${mingw}/lib/libgio-2.0.dll.a" obs _mingw32_cmake macro defines some more variables, see https://build.opensuse.org/package/view_file/windows:mingw:win32/mingw32-filesystem/macros.mingw32?expand=1 which let cmake be able to find libraries by itself if present. Comment on attachment 120231 [details] [review] [06/10] _dbus_test_oom_handling: allow disabling it as documented Review of attachment 120231 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 120232 [details] [review] [07/10] Do not require systemd to have a service file if using it for activation Review of attachment 120232 [details] [review]: ----------------------------------------------------------------- Looks good as far as I can say (I am not a systemd expert) ::: bus/activation.c @@ +1429,5 @@ > { > BusPendingActivation *p = _dbus_hash_iter_get_value (&iter); > > + if (p != pending_activation && p->exec != NULL && > + strcmp (p->exec, pending_activation->exec) == 0) make sense to avoid crashes on strcmp. @@ +1879,2 @@ > { > + pending_activation->exec = _dbus_strdup (entry->exec); no need to check entry->exec != 0 similar to line 1432 ? Created attachment 120245 [details] [review] add travis-ci.org build machinery The idea is that .travis.yml is specific to Travis-CI, but most of the actual work is done in tools/ci-build.sh, which should be reasonably CI-platform-agnostic (it currently assumes that build-dependendencies are preinstalled, that the "native" platform we're building on is GNU/Linux or something very close, and that "mingw" means mingw-w64 as packaged in Debian and Ubuntu). Bug: The following fixes have been pushed: 072d3a6 Don't try to run manual tests in installcheck 81f48a9 Fix shell syntax for installcheck-local with no DESTDIR 1b1d251 _dbus_test_oom_handling: allow disabling it as documented 9c3034f cmake: run all automated tests with --tap for better diagnostics b210934 embedded tests: accept and ignore --tap argument 4ddf4be cmake: copy the systemd-activation directory too 90a5da3 cmake: manual-tcp is not an automated test 3294b7a Avoid -Wunused-label when compiling with libselinux but no libaudit Created attachment 120251 [details] [review] add travis-ci.org build machinery The idea is that .travis.yml is specific to Travis-CI, but most of the actual work is done in tools/ci-build.sh, which should be reasonably CI-platform-agnostic (it currently assumes that build-dependendencies are preinstalled, that the "native" platform we're building on is GNU/Linux or something very close, and that "mingw" means mingw-w64 as packaged in Debian and Ubuntu). Bug: https://bugs.freedesktop.org/show_bug.cgi?id=93194 Reviewed-by: Ralf Habacker <ralf.habacker@freenet.de> Created attachment 120252 [details] [review] Don't try to run manual tests in installcheck We were mistakenly running all installed executables, even manual tests that never terminate. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=93194 Reviewed-by: Ralf Habacker <ralf.habacker@freenet.de> Created attachment 120253 [details] [review] Fix shell syntax for installcheck-local with no DESTDIR A closing brace must be preceded by a semicolon. The CI integration added later in this branch actually runs "make installcheck" with no DESTDIR; apparently nobody else has ever tried that. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=93194 Reviewed-by: Ralf Habacker <ralf.habacker@freenet.de> Created attachment 120254 [details] [review] Do not require systemd to have a service file if using it for activation With --systemd-activation we special-case the name org.freedesktop.systemd1 by assuming that it will eventually connect to the bus. With that in mind, we can ignore whether it has a .service file, and let it be "activated" regardless. This fixes a regression test failure on non-systemd systems such as the Ubuntu 14.04 OS on travis-ci.org: UpdateActivationEnvironment failed, because it tried to update the (fake) systemd environment, but because systemd was not actually installed, there was no service file for it in the system's search paths. We could address this by placing a dummy service file with Exec=/bin/false in our search path like the real systemd does, but it seems cleaner to not require this; this would eventually enable the real systemd to stop installing that dummy service file. This would not happen outside the regression tests, because there is no sense in using --systemd-activation without systemd installed. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=93194 Reviewed-by: Ralf Habacker <ralf.habacker@freenet.de> Created attachment 120255 [details] [review] _dbus_test_oom_handling: allow disabling it as documented We documented DBUS_TEST_MALLOC_FAILURES=0 in HACKING, but it didn't actually work: we'd iterate from i=-1 to i=0. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=93194 Reviewed-by: Ralf Habacker <ralf.habacker@freenet.de> Created attachment 120256 [details] [review] cmake: run all automated tests with --tap for better diagnostics For GLib-based tests it's useful, because it means g_test_message() gets logged. For the embedded tests it's now accepted and ignored. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=93194 Reviewed-by: Ralf Habacker <ralf.habacker@freenet.de> Created attachment 120257 [details] [review] embedded tests: accept and ignore --tap argument This makes them semi-command-line-compatible with a way we can invoke the GLib-based tests to get more useful debug logs. These tests still do not actually produce TAP output yet; I tried implementing that, but it requires changing a lot of noise on stdout to come out of stderr, and there was something weird going on with subprocesses restarting the test numbering which will need further investigation before making that change. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=93194 Reviewed-by: Ralf Habacker <ralf.habacker@freenet.de> Created attachment 120258 [details] [review] cmake: copy the systemd-activation directory too It is needed for a couple of test-cases. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=93194 Reviewed-by: Ralf Habacker <ralf.habacker@freenet.de> Created attachment 120259 [details] [review] cmake: manual-tcp is not an automated test Don't run it when we run automated tests. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=93194 Reviewed-by: Ralf Habacker <ralf.habacker@freenet.de> Created attachment 120260 [details] [review] Avoid -Wunused-label when compiling with libselinux but no libaudit Bug: https://bugs.freedesktop.org/show_bug.cgi?id=93194 Reviewed-by: Ralf Habacker <ralf.habacker@freenet.de> (In reply to Ralf Habacker from comment #22) > The following fixes have been pushed: This came from git bz push, which is good. Also all related patches are made obsolate, which is also good. Unfortunally git bz push also reattaches all patches to this bug, which is undocumented and bad :-( From http://git.fishsoup.net/man/git-bz.html: "... git bz push This does git push, adds a comment that the commits were pushed and marks the patches committed. The changes it is making to the bug will be shown in your editor to give you a chance to confirm them and add extra comments if desired. ..." It is also good, that I can make single patches obsolate by uncommenting the related line. Unfortunally the same does not work on pushing. I tried to exclude patch 7 and 10 because of issues. That means additional fixes needs to be committed as followup. (In reply to Ralf Habacker from comment #33) > Unfortunally git bz push also reattaches all patches to this bug, This "feature" has been added later http://blog.fishsoup.net/2009/09/05/git-bz-push/ " git bz push --fix 43215 This combines ‘git bz attach’ and ‘git bz push’ into one. So, to review, what it does is: Rewrites the commit to include the bug URL Attaches the commit to the bug, and marks it ‘committed’ Pushes the commit Adds a comment to the bug and mark it resolved " (In reply to Ralf Habacker from comment #18) > Note: x86_64 mingw target is missing Yes. I don't want to introduce too many parallel builds right now, and a 32-bit Windows build seems higher priority to test than 64-bit. I'll consider > Question: How are these builds triggered, on git HEAD commit hash change ? On any push (or pull request, but please don't send me pull requests) to smcv/dbus-mirror, or to any other clone of dbus.git on GitHub that has opted in to travis-ci integration. > Looks like a minimal setting, obs uses more variables Right, what I'm interested in here is the minimum to get a working build. > > + # don't run tests yet, Wine needs Xvfb > > You also need a wine prefix. on obs I use this > https://build.opensuse.org/package/show/home:rhabacker:branches:windows: > mingw:win32/mingw32-cross-wine I'm not adding tests under Wine right now. (In reply to Ralf Habacker from comment #20) > > + pending_activation->exec = _dbus_strdup (entry->exec); > > no need to check entry->exec != 0 similar to line 1432 ? No. A valid BusActivationEntry (essentially a struct representing a .service file) always has exec != NULL. The change made by this commit is to BusPendingActivation, the struct representing an activatable service that we are about to start, or that we have started but are still waiting for it to claim its bus name. The change is: * previously, a BusPendingActivation was always created for a non-NULL BusActivationEntry, therefore it had a non-NULL exec * now, the BusPendingActivation for "org.freedesktop.systemd1" is created without a BusActivationEntry, so it has a NULL exec (In reply to Ralf Habacker from comment #22) > The following fixes have been pushed: > 072d3a6 Don't try to run manual tests in installcheck > 81f48a9 Fix shell syntax for installcheck-local with no DESTDIR > 1b1d251 _dbus_test_oom_handling: allow disabling it as documented > 9c3034f cmake: run all automated tests with --tap for better diagnostics > b210934 embedded tests: accept and ignore --tap argument > 4ddf4be cmake: copy the systemd-activation directory too > 90a5da3 cmake: manual-tcp is not an automated test > 3294b7a Avoid -Wunused-label when compiling with libselinux but no libaudit In general I would prefer to push my own changes after review, particularly if you queried things during review and I have not yet responded. If you review a self-contained bug fix that I've written, and you have no queries or objections, it's OK to just push it, particularly if it's fixing something that was breaking the build; but for "features" originating from a maintainer, I'd really prefer for the author of the branch to be the one to confirm that it's ready and push it. (In reply to Simon McVittie from comment #35) > > Question: How are these builds triggered, on git HEAD commit hash change ? > > On any push (or pull request, but please don't send me pull requests) to > smcv/dbus-mirror, or to any other clone of dbus.git on GitHub that has opted > in to travis-ci integration. How are you updating the GitHub mirror? A cronjob somewhere which pushes to it? You might want to consider using phabricator.freedesktop.org for this — it has an automatic git repository mirroring function which pushes to remote mirrors of any repository which you’ve added to Phabricator (if you ask it to). (In reply to Philip Withnall from comment #37) > How are you updating the GitHub mirror? A cronjob somewhere which pushes to > it? At the moment, by hand, which is better than nothing. > You might want to consider using phabricator.freedesktop.org for this — it > has an automatic git repository mirroring function which pushes to remote > mirrors of any repository which you’ve added to Phabricator (if you ask it > to). If I understand correctly, that would require me to give phabricator.freedesktop.org control over my GitHub account, which I certainly don't want. Or does Phab already have its own GitHub account which I can add as a collaborator? (In reply to Simon McVittie from comment #38) > (In reply to Philip Withnall from comment #37) > > How are you updating the GitHub mirror? A cronjob somewhere which pushes to > > it? > > At the moment, by hand, which is better than nothing. > > > You might want to consider using phabricator.freedesktop.org for this — it > > has an automatic git repository mirroring function which pushes to remote > > mirrors of any repository which you’ve added to Phabricator (if you ask it > > to). > > If I understand correctly, that would require me to give > phabricator.freedesktop.org control over my GitHub account, which I > certainly don't want. Or does Phab already have its own GitHub account which > I can add as a collaborator? You can add a ‘deploy key’ to your project in GitHub, which is a separate SSH key for pushing to the repository. Give that SSH key to Phabricator for mirroring. (In reply to Philip Withnall from comment #39) > (In reply to Simon McVittie from comment #38) > to Phabricator for mirroring. So it's time to open dbus project on phabricator.freedesktop.org ? (In reply to Ralf Habacker from comment #40) > (In reply to Philip Withnall from comment #39) > > (In reply to Simon McVittie from comment #38) > > > to Phabricator for mirroring. > > So it's time to open dbus project on phabricator.freedesktop.org ? Iff you want to use the mirroring feature of Phabricator. (In reply to Philip Withnall from comment #41) > (In reply to Ralf Habacker from comment #40) > > (In reply to Philip Withnall from comment #39) > > > (In reply to Simon McVittie from comment #38) > > > > > to Phabricator for mirroring. > > > > So it's time to open dbus project on phabricator.freedesktop.org ? > > Iff you want to use the mirroring feature of Phabricator. Yes, I created a related project https://phabricator.freedesktop.org/project/profile/67/ Unfortunally git repo setup is limited to site admins.:-( Creating repo at https://phabricator.freedesktop.org/diffusion/ returns: You do not have permission to create new repositories. (In reply to Ralf Habacker from comment #42) > Creating repo at https://phabricator.freedesktop.org/diffusion/ returns: > > You do not have permission to create new repositories. Right. We would have to discuss this with the freedesktop.org sysadmins. Please don't create any more "official-looking" infrastructure without some discussion on the mailing list first: if we do this, I want to do it right. The stuff on GitHub and travis-ci is deliberately under my personal namespace, and marked as unofficial and subject to change/breakage; if we create things on fd.o infrastructure in particular, they're going to look "official" and people will start using them. (In particular, I really don't want to get into a situation where some people open D-Bus bugs as Bugzilla bugs and some people open them as Phabricator tasks!) (In reply to Simon McVittie from comment #43) > (In reply to Ralf Habacker from comment #42) > > Creating repo at https://phabricator.freedesktop.org/diffusion/ returns: > > > > You do not have permission to create new repositories. > > Right. We would have to discuss this with the freedesktop.org sysadmins. .o/ I have those powers on phab.fdo. > Please don't create any more "official-looking" infrastructure without some > discussion on the mailing list first: if we do this, I want to do it right. > The stuff on GitHub and travis-ci is deliberately under my personal > namespace, and marked as unofficial and subject to change/breakage; if we > create things on fd.o infrastructure in particular, they're going to look > "official" and people will start using them. > > (In particular, I really don't want to get into a situation where some > people open D-Bus bugs as Bugzilla bugs and some people open them as > Phabricator tasks!) Definitely. For libnice, we have the problem at the moment that someone will report a bug via the mailing list. Then they’ll find the GitHub and fork that and send a pull request. Then they’ll find Phabricator and create a task and diff there too. It’s a pain. (In reply to Simon McVittie from comment #43) > (In reply to Ralf Habacker from comment #42) > > Creating repo at https://phabricator.freedesktop.org/diffusion/ returns: > > > > You do not have permission to create new repositories. > > Right. We would have to discuss this with the freedesktop.org sysadmins. > > Please don't create any more "official-looking" infrastructure without some > discussion on the mailing list first: if we do this, I want to do it right. You did take a look at the dbus project on phabricator.freedesktop.org ? It is not public, noone can join and noone except the current members (which are you, Philip and I) can create tasks assigned to dbus :-) (In reply to Simon McVittie from comment #43) > > Right. We would have to discuss I filed a related bug at 93283. (In reply to Ralf Habacker from comment #46) > (In reply to Simon McVittie from comment #43) > > > > Right. We would have to discuss > I filed a related bug at 93283. bug 93283 This landed a while ago. For dbus 1.10.x, the CI glue is on a separate branch (dbus-1.10-ci), although I'm increasingly tempted to just merge it. (In reply to Simon McVittie from comment #48) > For dbus 1.10.x, the CI glue is on a separate > branch (dbus-1.10-ci), although I'm increasingly tempted to just merge it. This CI glue is sufficiently useful that I think we should bend the stable-branch policy enough to include it, so I've merged dbus-1.10-ci into dbus-1.10 (currently only on Github due to Bug #97014). The only "real" code change (not in build or test code) is Attachment #120232 [details]. I think that's minimal enough that we can just ship it. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.