712475 - ffprocess has several file utility functions that dont really belong there

Status: RESOLVED WONTFIX

(Testing :: Talos, defect)

Description

[Jeff Hammel]

Looking at
http://hg.mozilla.org/build/talos/file/fd70c47121be/talos/ffprocess_linux.py
as an example, there is

- MakeDirectoryContentsWritable(self, dirname)
- copyFile(self, fromfile, toDir)
- removeDirectory(self, dir)
- getFile(self, handle, localFile = "")

These are really generic utility functions that should go in utils.py
or in a file utility package, e.g. mozfile:
https://wiki.mozilla.org/Auto-tools/Projects/MozBase#shared_with_mozharness

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 712475 - ffprocess has several file utility functions that don't really belong there

Moved four methods (getFile, MakeDirectoryContentsWritable, removeDirectory, and copyFile) from platform specific ffprocess modules (win32, linux, mac, and remote) to utils.py.

Coded a special case in each for remote testing (this may be undesirable, could also check this at each time these methods are called).

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 588495 [details] [diff] [review]
Bug  712475 - ffprocess has several file utility functions that don't really belong there

Review of attachment 588495 [details] [diff] [review]:
-----------------------------------------------------------------

Overall I think this really does cover all the cases, but the remote stuff is confusing.  Would it be so bad to leave the remote stuff in place and special case remote vs desktop?  Ideally we could have mozdevice handle this and maybe work with utils.py somehow?  Maybe as a subclass?

If it wasn't for the remote stuff I think this would be a r+, but we should revisit this patch to handle remote cases properly.

::: talos/bcontroller.py
@@ -94,5 @@
>        else:
>          remoteLog = devroot + '/' + self.browser_log.split('/')[-1]
>          retVal = self.remoteProcess.launchProcess(self.command, outputFile=remoteLog, timeout=self.test_timeout)
>          if retVal <> None:
> -          self.remoteProcess.getFile(retVal, self.browser_log)

this is a remote specific case, we should leave it alone for now.

::: talos/ffprocess_remote.py
@@ -174,5 @@
> -            localFile = os.path.join(tempdir, "temp.txt")
> -            temp = True
> -
> -        re_nofile = re.compile("error:.*")
> -        data = self.testAgent.getFile(handle, localFile)

this is the magic clode that isn't in the utils.py stuff.  We might want to leave all this remote stuff here for now.

@@ -222,5 @@
>              raise talosError("Unable to copy '%s' to remote device '%s'" % (localDir, remoteDir))
>          return remoteDir
> -  
> -    def removeDirectory(self, dir):
> -        if (self.testAgent.removeDir(dir) is None):

utils is using shutil and this won't work as is on a remote system.

::: talos/ffsetup.py
@@ -207,5 @@
>          temp_dir = tempfile.mkdtemp()
>          profile_dir = os.path.join(temp_dir, 'profile')
>          shutil.copytree(source_profile, profile_dir)
> -        self._hostproc.MakeDirectoryContentsWritable(profile_dir)
> -

this will work fine

::: talos/remotePerfConfigurator.py
@@ -136,5 @@
>              else:
>                parts = self.exePath.split('/')
>                remoteFile = '/'.join(parts[0:-1]) + '/' + self.masterIniSubpath
>              if (not os.path.isfile(localfilename)):
> -              filecontents = self.testAgent.getFile(remoteFile, localfilename)

this is getting a file from a remote device, we should leave this alone for now.

::: talos/run_tests.py
@@ -374,5 @@
>          remoteAppIni = '/data/data/' + browser_config['browser_path'] + '/' + appIniFileName
>        else:
>          remoteAppIni = browser_config['deviceroot'] + '/' + appIniFileName
>        if (not os.path.isfile('remoteapp.ini')):
> -        devicemanager.getFile(remoteAppIni, 'remoteapp.ini')

this is remote only, we might want to leave this alone or figure out a cleaner interface in mozdevice.

::: talos/ttest.py
@@ +314,5 @@
>                  while total_time < timeout:
>                      # Sleep for [resolution] seconds
>                      time.sleep(resolution)
>                      total_time += resolution
> +                    fileData = utils.getFile(b_log)

I suspect this will fail on mobile

::: talos/utils.py
@@ +46,5 @@
>  DEBUG = 0
>  NOISY = 0
>  START_TIME = 0
>  saved_environment = {}
> +remote_ = platform.system() not in ("Windows", "Microsoft", "Darwin", "Linux")

hmm, this won't work to detect remote since the code is running on a host machine and sending remote calls.

@@ +175,5 @@
> +        pass
> +    else:
> +        try:
> +            for (root, dirs, files) in os.walk(dirname):
> +                os.chmod(root, 0755)

win32 uses 0777

@@ +178,5 @@
> +            for (root, dirs, files) in os.walk(dirname):
> +                os.chmod(root, 0755)
> +                for filename in files:
> +                    try:
> +                        os.chmod(os.path.join(root, filename), 0755)

win32 uses 0777

@@ +193,5 @@
> +    """
> +
> +    if not "remote_":
> +        self.MakeDirectoryContentsWritable(dir)
> +    shutil.rmtree(dir)

we need to determine a method for connecting to the remote device and removing files via the mozdevice interface.

@@ +198,5 @@
> +
> +def copyFile(fromfile, toDir):
> +    if remote_:
> +        toDir = toDir.replace("/", self.dirSlash) 
> +        if (RemoteProcess.testAgent.pushFile(fromfile, toDir + self.dirSlash + os.path.basename(fromfile)) is False):

where is RemoteProcess defined?

Comment 3

[William Lachance (:wlach)]

I'm not sure if the premise of this bug is quite correct. Looking at the code, it looks to me like copyFile, removeDirectory, and getFile methods basically exist to abstract away the differences between running Firefox locally (i.e. desktop, where talos is running on the same machine as the browser) vs. remotely (i.e. mobile, where talos is running on a different machine than the browser). This is a good thing, though I do agree the implementation could be a bit better. For example, it's not really clear to me why copyFile doesn't just call shutil.copy directly for the Linux, Mac, and Windows cases.

As for MakeDirectoryContentsWritable... AFAICT, this method only really exists so that shutil.rmtree will work under windows. It is not needed on Android, Linux, or Mac (there is a small routine in CreateTempProfileDir in ffsetup.py which uses it, but I don't think that actually does anything). We might just want to inline this method in ffprocess_win32.removeDirectory. See: http://stackoverflow.com/a/1214935 

Regarding Joel's question here:

"Overall I think this really does cover all the cases, but the remote stuff is confusing.  Would it be so bad to leave the remote stuff in place and special case remote vs desktop?  Ideally we could have mozdevice handle this and maybe work with utils.py somehow?  Maybe as a subclass?"

I'm not sure if that makes sense: effectively mozdevice/devicemanager is already the layer of abstraction you're talking about here in the sense that it provides an API for file manipulation that works with a remotely connected Android device. IMO, the only way to do better here would be to move up one level of abstraction to that of actually running Firefox (i.e. using mozprofile/mozrunner in Talos). We've already discussed that a bit here:

https://bugzilla.mozilla.org/show_bug.cgi?id=698898 (talos should use mozrunner)
https://bugzilla.mozilla.org/show_bug.cgi?id=697486 (variant of mozrunner for running fennec remotely, needed for the above)

If anyone's interested in taking on this work (either alone or as a group), I'd be happy to help.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

it appears that most of these changes have been added via other re-factoring bugs.  I am marking this as wontfix since the ffprocess_*.py files are much simpler now with little to no duplicated code.