GNOME Bugzilla – Bug 691568
Improve gvfs-test on Gentoo
Last modified: 2013-02-05 17:32:23 UTC
Since Gentoo is my primary development platform and is kinda different from mainstream distributions, I'll be posting patches adapting gvfs-test for specifics of this system.
Created attachment 233244 [details] [review] gvfs-test: Include */sbin in PATH My $PATH is currently set to "/usr/local/bin:/usr/bin:/bin:/opt/bin:/usr/x86_64-pc-linux-gnu/gcc-bin/4.7.2:/usr/lib64/subversion/bin:/usr/games/bin:/data/home/tbzatek/bin" lacking any variant of sbin, so let's add it specifically in run-in-tree.sh (btw. what about /opt/sbin ?)
Created attachment 233246 [details] [review] gvfs-test: Search for mkisofs when genisoimage is not found Some may prefer original cdrtools over forked cdrkit. Though cdrtools seems to be actively developed while cdrkit is rather stalled, arguments for mkisofs vs. genisoimage have not changed. Hopefully it stays that way in the future. This patch adds a simple function that takes an array of commands and tries to find the one present in running system.
Created attachment 233254 [details] [review] gvfs-test: Use apache2 binary directly when there's no apachectl Next in line is Apache. Ubuntu (testing on Linaro here, guessing it's all inherited from Debian anyway) tends to distribute both apachectl and apache2ctl while the former is just a symlink. Both support mixed SysV init and apache2 pass-through modes. The master binary is /usr/sbin/apache2 which is a symlink to chosen worker. Fedora only ships apachectl and supports both modes. The master binary is /usr/sbin/httpd. Gentoo only ships apache2ctl and it's effectively a symlink to openrc initscript, not understanding apache2 arguments. The master binary is /usr/sbin/apache2. Since the passthrough seems to be 1:1, we can use the master binary directly. But as long as its name is not universal, better to keep searching for apachectl and fall back to apache2 if not found. Not ideal but brings higher chance.
Comment on attachment 233244 [details] [review] gvfs-test: Include */sbin in PATH This seem fine to me. I'm not an official gvfs maintainer, but as author of the tests I'll be so bold to set this to ACN. :-)
Comment on attachment 233246 [details] [review] gvfs-test: Search for mkisofs when genisoimage is not found I recently changed the Drive tests to use pre-generated ISO9660 and ext2 images, which is much easier than trying to generate all of them during the tests without root privileges. These 1 MB raw images compress into nothing (half a kB), so I wonder if it wouldn't be better to ship the remaining missing ISO (the one with a Joliet extension) in tests/files/ as well, and entirely drop the cdrkit dependency?
Comment on attachment 233254 [details] [review] gvfs-test: Use apache2 binary directly when there's no apachectl Sorry, I thought apachectl was something upstreamish. Would there be anything wrong with just always calling "apache2"? That works fine on Debian/Ubuntu as well. Also, if we use this patch, I think all the comments and strings containing "apachectl" should be changed as well.
Comment on attachment 233246 [details] [review] gvfs-test: Search for mkisofs when genisoimage is not found As explained before, I think this is more robust and also more consistent with the Drive tests: http://git.gnome.org/browse/gvfs/commit/?id=44f9a55da370969578899390ff00ba2bd34fb95f
Comment on attachment 233244 [details] [review] gvfs-test: Include */sbin in PATH (In reply to comment #4) > (From update of attachment 233244 [details] [review]) > This seem fine to me. I'm not an official gvfs maintainer, but as author of the > tests I'll be so bold to set this to ACN. :-) Pushed as de3023888932176df348252b350a8651cc12520f
Created attachment 233529 [details] [review] gvfs-test: Use apache2 binary directly when there's no apachectl (In reply to comment #6) > (From update of attachment 233254 [details] [review]) > Sorry, I thought apachectl was something upstreamish. Yeah, that was my assumption as well at the beginning. > Would there be anything wrong with just always calling "apache2"? That works > fine on Debian/Ubuntu as well. That unfortunately doesn't work on Fedora/RHEL as the main binary is called httpd (historic reasons I would say?). > Also, if we use this patch, I think all the comments and strings containing > "apachectl" should be changed as well. Attaching updated patch, containing the find_alternative() method and modifying all other traces of 'apachectl'
Comment on attachment 233529 [details] [review] gvfs-test: Use apache2 binary directly when there's no apachectl Looks good, thanks! As a stylistic nitpick, this: + path = subprocess.call(['which', cmd], stdout=subprocess.PIPE) + if path == 0: is not actually returning a path, just an exit code. So perhaps if subprocess.call(...) == 0: return cmd
Created attachment 234015 [details] [review] gvfs-test: Handle missing ~/.ssh/id_rsa gracefully Not entirely related to cross-distro support, however I've been hit by this assert on a new homedir: Traceback (most recent call last):
+ Trace 231407
unittest.main(testRunner=unittest.TextTestRunner(stream=sys.stdout, verbosity=2))
self.runTests()
self.result = testRunner.run(self.test)
test(result)
return self.run(*args, **kwds)
print('\n----- sshd log -----\n%s\n------\n' % self.sshd.stderr.read())
make: *** [check] Error 1
Created attachment 234018 [details] [review] gvfs-test: Use apache2 binary directly (In reply to comment #6) > (From update of attachment 233254 [details] [review]) > Would there be anything wrong with just always calling "apache2"? That works > fine on Debian/Ubuntu as well. At the end, after installing Fedora 19 (rawhide) and testing there, I started getting the following message: Passing arguments to httpd using apachectl is no longer supported. You can only start/stop/restart httpd using this script. If you want to pass extra arguments to httpd, edit the /etc/sysconfig/httpd config file. which was done due to need of going through systemd to fix some issues: http://pkgs.fedoraproject.org/cgit/httpd.git/commit/?id=a62c50b2bed51e40a7f697199a6ab02bc166de69 So I've modified the patch once more and switch to using 'apache2' or 'httpd' directly when available, with a fallback to old 'apachectl' in case of other/legacy systems before we fail completely. (In reply to comment #10) > (From update of attachment 233529 [details] [review]) > Looks good, thanks! > > As a stylistic nitpick, this: > > + path = subprocess.call(['which', cmd], stdout=subprocess.PIPE) > + if path == 0: > > is not actually returning a path, just an exit code. So perhaps > > if subprocess.call(...) == 0: > return cmd Thanks for spotting this, I originally thought we'd use full path, this was just a leftover. Removed.
Created attachment 234020 [details] [review] gvfs-test: Add more sftp-server search paths A tiny patch to include Gentoo-specific path to sftp-server binary. I hate hardcoded paths but have no better idea since sftp service is usually hidden by ssh server.
Created attachment 234021 [details] [review] gvfs-test: Count error numbers properly I've been getting tracebacks at various places, seems like objects/lists were used directly instead of counting number of elements. Inspired elsewhere in the code, I've made this patch, please check. Traceback (most recent call last):
+ Trace 231408
if result and result.errors + result.failures > orig_err_fail and hasattr(self, "sshd"):
Created attachment 234035 [details] [review] gvfs-test: Fix private dir in generated smb.conf This patch makes Smb tests working on samba 3.x series. Still getting some auth issues, need to look at that more.
Comment on attachment 234018 [details] [review] gvfs-test: Use apache2 binary directly Thanks for the update! >+def find_alternative(cmds): >+ '''Tries to find commands in cmds array and returns an alternative found''' >+ for cmd in cmds: For consistency, can you please leave a blank line after the docstring, and also write it in the nominative? "Find command in cmds array and return the found alternative". Looks good to me otherwise.
Comment on attachment 234015 [details] [review] gvfs-test: Handle missing ~/.ssh/id_rsa gracefully Nice catch, thanks! Can you please use single quotes in the hasattr() call for consistency?
Comment on attachment 234020 [details] [review] gvfs-test: Add more sftp-server search paths That's straightforward.
Comment on attachment 234021 [details] [review] gvfs-test: Count error numbers properly Oh indeed, GvfsTestCase.run() already does it correctly, but not the subclasses. Thanks!
Comment on attachment 234035 [details] [review] gvfs-test: Fix private dir in generated smb.conf I confirm that this works on 3.9 as well. Back then I tried to use one variant only, but there were some settings which only worked as "dir" and some others which only worked as "directory". Yay naming. Thanks!
(In reply to comment #17) > (From update of attachment 234015 [details] [review]) > Nice catch, thanks! Can you please use single quotes in the hasattr() call for > consistency? Fixed. Thanks for watching a Python lame :-) (In reply to comment #16) > (From update of attachment 234018 [details] [review]) > For consistency, can you please leave a blank line after the docstring, and > also write it in the nominative? "Find command in cmds array and return the > found alternative". Changed those two as well. (In reply to comment #20) > (From update of attachment 234035 [details] [review]) > I confirm that this works on 3.9 as well. Back then I tried to use one variant > only, but there were some settings which only worked as "dir" and some others > which only worked as "directory". Yay naming. Yes, it's inconsistent and while both alternatives are usually available for other keys, this one is not unfortunately. Committed all patches to master.
(In reply to comment #15) > This patch makes Smb tests working on samba 3.x series. Still getting some auth > issues, need to look at that more. Those were not auth issues at the end but troubles unmounting the backend. The mount phase and all tests ran successfully. Normally unmounting takes some time and the backend may be around for a second or two more. That's why GvfsTestCase.unmount() allows up to five seconds delay which is correct. But in my case (on one of three tested systems) the backend stays active-stuck after "gvfs-mount -u" has been issued with the following backtrace:
+ Trace 231431
It's waiting for the nc command (tunnelling smbd) to finish but the very weird thing is that it's spawned from do_mount()... which leads me to assumption that libsmbclient forked the process (it's shown in "ps" for a short period of time) and made quite a mess. Daemon log shows gvfs calls including successful GVfsJobUnmount. Keeping this bugreport open until we sort this issue out.
I've also seen some (random) test failures on first run when my computer was swappy or under load (not to mention running in a VM). Second run always succeeded. We may probably need to tweak delays between daemon spawn and particular tests as I think the daemons may need more time to start (e.g. twistd, smbd). I understand that subprocess.Popen() is async and often we would have to rely on some stdout/stderr string printed out to indicate the daemon is up (this would be tough for different smbd versions). Just an idea.
Created attachment 234632 [details] [review] daemon: Implement proper org.gtk.vfs.MountTracker.UnregisterMount() This patch works around the SMB issue I was having but apparently breaks other things as some tests started failing. I will look at that more, but first, review of bug 691336 is waiting.
Created attachment 234750 [details] [review] gvfs-test: Don't wait for GVolumeMonitor "mount-removed" signal (In reply to comment #24) > This patch works around the SMB issue I was having but apparently breaks other > things as some tests started failing. I will look at that more, but first, > review of bug 691336 is waiting. This patch fixes the phenomenon above, it's actually a problem in tests, a workaround that is not needed anymore.
Comment on attachment 234750 [details] [review] gvfs-test: Don't wait for GVolumeMonitor "mount-removed" signal I thought I added that "wait for mount removed" as otherwise I got race conditions with the next test, but I could be wrong. Does it run reliably with this patch? I'll test this here tomorrow morning.
Comment on attachment 234750 [details] [review] gvfs-test: Don't wait for GVolumeMonitor "mount-removed" signal > I thought I added that "wait for mount removed" as otherwise I got race > conditions with the next test Indeed, I get failures with this patch: test_anonymous_api (__main__.Ftp) ftp:// anonymous (API) ... ok test_anonymous_cli (__main__.Ftp) ftp:// anonymous (CLI) ... Error mounting location: Location is already mounted ERROR That's why I added it in the first place -- when unmount_done() is called, the mount hasn't really disappeared yet, but waiting for mount_removed seems to do the trick for me. Or is that what your previous patch ("Implement proper org.gtk.vfs.MountTracker.UnregisterMount()") actually fixes, i. e. when unmount_with_operation() calls the callback the mount is _really_ gone?
(In reply to comment #26) > (From update of attachment 234750 [details] [review]) > I thought I added that "wait for mount removed" as otherwise I got race > conditions with the next test, but I could be wrong. Does it run reliably with > this patch? Oh, I should've mention the last two patches are related. Yes, it seems to run reliably. If not, then it should be treated as error. I.e. once the unmount operation finishes, the mount should not be listed and immediate mounting it again should work. The only thing that can fail I can think of is "mount-removed" signal delivery that is async. I.e. the computer backend relies on that but I guess nobody is using it seriously. (In reply to comment #27) > Or is that what your previous patch ("Implement proper > org.gtk.vfs.MountTracker.UnregisterMount()") actually fixes, i. e. when > unmount_with_operation() calls the callback the mount is _really_ gone? Exactly.
Comment on attachment 234750 [details] [review] gvfs-test: Don't wait for GVolumeMonitor "mount-removed" signal Pushed asb9c3d2c11c854ca9c3b173fe8cebab21bfaa46d4
Comment on attachment 234632 [details] [review] daemon: Implement proper org.gtk.vfs.MountTracker.UnregisterMount() Pushed as 62ba3def657a053c0a8aa805e7e3fe3a25d3759d
I went ahead and committed both patches as they proved to be reliable over dozens of tests ran here while debugging other things. Closing this bugreport too since all issues have been fixed and no more distro-specific things are hanging in the air at the moment.