Closed Bug 795071 Opened 9 years ago Closed 9 years ago

update sut_tools to use a more modern logging infrastructure

Categories

(Release Engineering :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jmaher, Assigned: jmaher)

References

Details

Attachments

(1 file, 2 obsolete files)

The sut_tools could use a more involved logging system to replace "print" commands.
what the title of the attachment says.
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
Attachment #665587 - Flags: review?(bugspam.Callek)
Comment on attachment 665587 [details] [diff] [review]
update sut_tools to use log.* functions instead of print (1.0)

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

::: sut_tools/sut_lib.py
@@ +23,5 @@
>  import json
>  
> +def getSUTLogger(filename=None, loggername=None):
> +    if not filename:
> +        filename = "sut_tools.log"

If nothing else, I need/want a way to not have the commands run log to a file, for when buildbot calls stuff.

I know you're not great at logging stuff, and I'm not a fan of this sut_lib code, I'll see if I can make a pass at this building upon your log.* changes elsewhere, unless you think you can cleanup at the least the filename stuff in a way we can try to land this and get the improvement earlier.
Attachment #665587 - Flags: review?(bugspam.Callek) → review-
update to not default to a file.
Attachment #665587 - Attachment is obsolete: true
Attachment #666584 - Flags: review?(bugspam.Callek)
Comment on attachment 666584 [details] [diff] [review]
update sut_tools to use log.* functions instead of print (1.1)

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

::: sut_tools/reboot.py
@@ +10,5 @@
>  
> +def reboot(dm):
> +    cwd       = os.getcwd()
> +    proxyFile = os.path.join(cwd, '..', 'proxy.flg')
> +    errorFile = os.path.join(cwd, '..', 'error.flg')

warning this still has a pretty strict requirement on what the cwd is for the flags to be useful outside of this file -- but it relies on this in both the before and after of your patch, so just an observation

@@ +28,5 @@
> +            waitForDevice(dm, waitTime=600)
> +        except SystemExit:
> +            clearFlag(proxyFile)
> +            setFlag(errorFile, "Remote Device Error: call for device reboot failed")
> +            sys.exit(1)

not required, but we might as well move this sys.exit() to be a return 1 and handled like other sut_* scripts are these days.

::: sut_tools/sut_lib.py
@@ +39,5 @@
> +                            datefmt='%m/%d/%Y %H:%M:%S')
> +    else:
> +        logging.basicConfig(level=logging.DEBUG,
> +                            filename=filename,
> +                            filemode="a", 

nit: lets use **kwargs magic here.

So something like:
extra_kwagrs = {}
if filename:
  extra_kwargs['filename'] = filename
  extra_kwargs['filemode'] = "a"

logging.basicConfig(...
                    **extra_kwargs)

And we do want stream output in both the file and the default logging.
Attachment #666584 - Flags: review?(bugspam.Callek) → review+
updated to include review nits.
Attachment #666584 - Attachment is obsolete: true
Attachment #666647 - Flags: review+
Hrm we broke some here:

+ for i in '${tegras}'
+ '[' -d tegra-013 ']'
+ cd tegra-013
+ '[' '!' -e clientproxy.pid ']'
+ python clientproxy.py -b --tegra=tegra-013 --debug
No handlers could be found for logger "multiprocessing"
+ sleep 60

Though it does seem to still be logging, so we might just be missing some early logging related to that.
Blocks: 797697
http://hg.mozilla.org/build/tools/rev/8b0a348b7f59
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.