Closed
Bug 996862
Opened 11 years ago
Closed 11 years ago
devicemanager getLanIp relies on a list of expected network interface names
Categories
(Testing :: Mozbase, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla31
People
(Reporter: gbrown, Assigned: wlach)
References
Details
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
wlach
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ahal
:
review+
|
Details | Diff | Splinter Review |
bjacob had trouble running remote reftests today. He was getting this error:
bjacob:/hack/mozilla-central$ MOZ_HOST_BIN=/hack/mozilla-central/obj-firefox-debug/dist/bin/ TEST_PATH=layout/reftests/bugs/reftest.list make -C obj-mobile-debug/ reftest-remote
make: Entering directory `/hack/mozilla-central/obj-mobile-debug'
ERROR: Either you specified the loopback for the remote webserver or your local IP cannot be detected. Please provide the local ip in --remote-webserver
ERROR: Invalid options specified, use --help for a list of valid options
/bin/sh: line 11: @errors=: command not found
make: Leaving directory `/hack/mozilla-central/obj-mobile-debug'
which seems to be caused by getLanIp failing because none of bjacob's network interface names matches the list at:
http://hg.mozilla.org/mozilla-central/annotate/1f932e462b84/testing/mozbase/mozdevice/mozdevice/devicemanager.py#l601
if (ip is None or ip.startswith("127.")) and os.name != "nt":
interfaces = ["eth0","eth1","eth2","wlan0","wlan1","wifi0","ath0","ath1","ppp0"]
There must be a better way to do this.
Reporter | ||
Comment 1•11 years ago
|
||
Comment 2•11 years ago
|
||
Wait, don't we have code to do this already?
http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/moznetwork/moznetwork/moznetwork.py
I'm guessing devicemanager was written before moznetwork and was never updated to use the new library.
Assignee | ||
Comment 3•11 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #2)
> Wait, don't we have code to do this already?
> http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/moznetwork/
> moznetwork/moznetwork.py
>
> I'm guessing devicemanager was written before moznetwork and was never
> updated to use the new library.
Yes, correct. We should probably turn getLanIp() into a compatibility shim around moznetwork.
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to William Lachance (:wlach) from comment #3)
> (In reply to Andrew Halberstadt [:ahal] from comment #2)
> > Wait, don't we have code to do this already?
> > http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/moznetwork/
> > moznetwork/moznetwork.py
> >
> > I'm guessing devicemanager was written before moznetwork and was never
> > updated to use the new library.
>
> Yes, correct. We should probably turn getLanIp() into a compatibility shim
> around moznetwork.
And I guess in this case, just replace calls to getLanIp() (and any abstractions around it) with calls to moznetwork.
Assignee | ||
Comment 5•11 years ago
|
||
I'm going to take this one. I think the right way forward is to kill any usage of getLanIp(). As far as I know there are two users: the in-tree code (subject of this bug) and talos... I thought sut_tools might as well, but apparently not: https://hg.mozilla.org/build/tools/file/c290db3dd537/sut_tools/
(1) Fix in-tree code to use moznetwork
(2) Fix talos to use moznetwork
(3) Remove this functionality from mozdevice
Assignee: nobody → wlachance
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #8408472 -
Flags: review?(ahalberstadt)
Assignee | ||
Comment 7•11 years ago
|
||
Comment 8•11 years ago
|
||
Comment on attachment 8408472 [details] [diff] [review]
Remove getLanIp usage, replace with moznetwork
Review of attachment 8408472 [details] [diff] [review]:
-----------------------------------------------------------------
Is something still using NetworkTools? Any reason not to remove that as well?
::: build/mobile/b2gautomation.py
@@ -129,5 @@
>
> return app, args
>
> - def getLanIp(self):
> - nettools = NetworkTools()
You can get rid of the import too.
::: build/mobile/remoteautomation.py
@@ -175,5 @@
> # return app, ['--environ:NO_EM_RESTART=1'] + args
> return app, args
>
> - def getLanIp(self):
> - nettools = NetworkTools()
Same.
Attachment #8408472 -
Flags: review?(ahalberstadt) → review+
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #8408490 -
Flags: review?(jmaher)
Assignee | ||
Comment 10•11 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #8)
> Is something still using NetworkTools? Any reason not to remove that as well?
Just talos AFAIK (just wrote up a patch for that). I was planning to do up a seperate patch to remove from mozdevice.
Comment 11•11 years ago
|
||
Comment on attachment 8408490 [details] [diff] [review]
Remove use of mozdevice's networktools
Review of attachment 8408490 [details] [diff] [review]:
-----------------------------------------------------------------
r- for the need of error handling.
::: create_talos_zip.py
@@ +44,4 @@
> ('mozdevice/mozdevice/droid.py', 'mozdevice/droid.py')]
> mozdevice = [(mozdevice_src + src, destination) for src, destination in mozdevice_files]
>
> +moznetwork_src = 'https://raw.github.com/mozilla/mozbase/moznetwork-0.24/'
does this really live on github? I thought we were in tree
::: setup.py
@@ +18,4 @@
> 'mozhttpd == 0.5',
> 'mozinfo == 0.7',
> 'datazilla == 1.4',
> + 'moznetwork == 0.24',
does this live on pypi.mozilla.org (or whatever that server is for internal python packages)
::: talos/PerfConfigurator.py
@@ +436,5 @@
> if self.config.get('develop'):
> if not self.config.get('webserver'):
> + s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
> + s.bind(("127.0.0.1",0))
> + self.config['webserver'] = 'localhost:%s' % s.getsockname()[1]
what if there is an error here? can we try/except and terminate if we fail?
Attachment #8408490 -
Flags: review?(jmaher) → review-
Comment 12•11 years ago
|
||
Comment on attachment 8408490 [details] [diff] [review]
Remove use of mozdevice's networktools
I was beaten into submission on IRC and now have a r+.
Attachment #8408490 -
Flags: review- → review+
Assignee | ||
Comment 13•11 years ago
|
||
Removing now-unnecessary imports per review suggestion, carrying forward r+.
Attachment #8408472 -
Attachment is obsolete: true
Attachment #8409677 -
Flags: review+
Assignee | ||
Comment 14•11 years ago
|
||
Try run looks good (https://tbpl.mozilla.org/?tree=Try&rev=65744b48160c), please checkin patch 8409677 when there is a chance.
https://bug996862.bugzilla.mozilla.org/attachment.cgi?id=8409677
(leave this open for followup work)
Keywords: checkin-needed,
leave-open
Updated•11 years ago
|
Attachment #8408490 -
Attachment is obsolete: true
Assignee | ||
Comment 15•11 years ago
|
||
Comment on attachment 8408490 [details] [diff] [review]
Remove use of mozdevice's networktools
This one isn't obsolete, though it shouldn't be checked into m-c.
Attachment #8408490 -
Attachment is obsolete: false
Comment 16•11 years ago
|
||
Keywords: checkin-needed
Assignee | ||
Comment 17•11 years ago
|
||
Pushed the talos patch: https://hg.mozilla.org/build/talos/rev/cf992f0f3b22
Comment 18•11 years ago
|
||
Assignee | ||
Comment 19•11 years ago
|
||
And here's a patch to remove NetworkTools from mozdevice altogether. I don't think anything uses it anymore. In the unlikely event that there is, pretty trivial to update it to use moznetwork.
Attachment #8409793 -
Flags: review?(ahalberstadt)
Comment 20•11 years ago
|
||
Comment on attachment 8409793 [details] [diff] [review]
Remove NetworkTools from mozdevice
Review of attachment 8409793 [details] [diff] [review]:
-----------------------------------------------------------------
\o/ awesome, thanks for doing this!
Attachment #8409793 -
Flags: review?(ahalberstadt) → review+
Assignee | ||
Comment 21•11 years ago
|
||
Ok, let's land attachment 8409793 [details] [diff] [review] as well! I didn't do a try run, but I did run the mozbase unit tests and all seems to be good. Also a quick mxrs shows that we're no longer importing/using NetworkTools anywhere aside from inside mozdevice itself (which this patch addresses):
http://mxr.mozilla.org/mozilla-central/search?string=NetworkTools
http://mxr.mozilla.org/mozilla-central/search?string=getLanIp
Keywords: leave-open → checkin-needed
Comment 22•11 years ago
|
||
Keywords: checkin-needed
Comment 23•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•