Closed
Bug 996862
Opened 10 years ago
Closed 10 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)
4.88 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
5.09 KB,
patch
|
wlach
:
review+
|
Details | Diff | Splinter Review |
6.84 KB,
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•10 years ago
|
||
Possibly http://code.activestate.com/recipes/439093/?
Comment 2•10 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•10 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•10 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•10 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•10 years ago
|
||
Attachment #8408472 -
Flags: review?(ahalberstadt)
Assignee | ||
Comment 7•10 years ago
|
||
Try run: https://tbpl.mozilla.org/?tree=Try&rev=65744b48160c
Comment 8•10 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•10 years ago
|
||
Attachment #8408490 -
Flags: review?(jmaher)
Assignee | ||
Comment 10•10 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•10 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•10 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•10 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•10 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•10 years ago
|
Attachment #8408490 -
Attachment is obsolete: true
Assignee | ||
Comment 15•10 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•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0daa735e865
Keywords: checkin-needed
Assignee | ||
Comment 17•10 years ago
|
||
Pushed the talos patch: https://hg.mozilla.org/build/talos/rev/cf992f0f3b22
Assignee | ||
Comment 19•10 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•10 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•10 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•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b65cb4bc310f
Keywords: checkin-needed
Comment 23•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b65cb4bc310f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•