ffprocess has several file utility functions that dont really belong there

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
7 years ago
5 years ago

People

(Reporter: k0scist, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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
(Reporter)

Updated

7 years ago
Whiteboard: [mozbase][mozfile]
(Reporter)

Updated

7 years ago
Whiteboard: [mozbase][mozfile] → [mozbase][mozfile][good first bug][mentor=jhammel][lang=py]
Created attachment 588495 [details] [diff] [review]
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).
Attachment #588495 - Flags: review?(jmaher)
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?
Attachment #588495 - Flags: review?(jmaher) → review-
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.
(Reporter)

Updated

7 years ago
Whiteboard: [mozbase][mozfile][good first bug][mentor=jhammel][lang=py]
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.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.