GCLI actor fails to getRequisition for new B2G servers

RESOLVED FIXED in Firefox 40

Status

defect
RESOLVED FIXED
4 years ago
10 months ago

People

(Reporter: jryans, Assigned: jryans)

Tracking

({regression})

unspecified
Firefox 40
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [polish-backlog])

Attachments

(8 attachments, 1 obsolete attachment)

39 bytes, text/x-review-board-request
bgrins
: review+
jwalker
: review+
Details
39 bytes, text/x-review-board-request
bgrins
: review+
jwalker
: review+
Details
39 bytes, text/x-review-board-request
bgrins
: review+
jwalker
: review+
Details
39 bytes, text/x-review-board-request
bgrins
: review+
jwalker
: review+
Details
39 bytes, text/x-review-board-request
bgrins
: review+
jwalker
: review+
Details
39 bytes, text/x-review-board-request
bgrins
: review+
jwalker
: review+
Details
39 bytes, text/x-review-board-request
bgrins
: review+
jwalker
: review+
Details
39 bytes, text/x-review-board-request
bgrins
: review+
jwalker
: review+
Details
GCLI now seems to assume files from /browser are available on servers, but that is not true for non-Firefox.

For example, a recent B2G nightly simulator build gives:

"Protocol error (unknownError): Module `devtools/commandline/commands-index` is not found at resource:///modules/devtools/commandline/commands-index.js" protocol.js:20
"Message: Module `devtools/commandline/commands-index` is not found at resource:///modules/devtools/commandline/commands-index.js
  Stack:
    GcliActor<._getRequisition@resource://gre/modules/devtools/server/actors/gcli.js:12:209
GcliActor<.specs<@resource://gre/modules/devtools/server/actors/gcli.js:2:734
actorProto/</handler@resource://gre/modules/devtools/server/protocol.js:74:9
DSC_onPacket@resource://gre/modules/devtools/server/main.js:97:215
ChildDebuggerTransport.prototype.receiveMessage@resource://gre/modules/devtools/transport/transport.js:38:337
See Also: → 1157906
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Hmmm, the solution to this could be via bug 1156682.

Alternatively we just could pull commands-index.js into toolkit, but I fear that's a bit like saying "Just make the toString() asynchronous", or "I'll just pull at the thread in this jumper to tidy it up".

Could a solution to bug 1160345 also provide a temporary solution to this problem?
Actually, on reflection I think injecting modules (comment 1) isn't the right solution.

I we should move commands-index.js from /browser/devtools/commandline to /toolkit/devtools/gcli and then wrap line 82 [1] in a try/catch (because that's where it all starts unravelling) and it might just work.

We may need to move command files into toolbox, and then hard-code them in commands-index, to make them available to b2g servers, but that can then be done one by one if needed.

[1]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/commands-index.js#82
Blocks: 1160559
(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #2)
> Actually, on reflection I think injecting modules (comment 1) isn't the
> right solution.
> 
> I we should move commands-index.js from /browser/devtools/commandline to
> /toolkit/devtools/gcli and then wrap line 82 [1] in a try/catch (because
> that's where it all starts unravelling) and it might just work.
> 
> We may need to move command files into toolbox, and then hard-code them in
> commands-index, to make them available to b2g servers, but that can then be
> done one by one if needed.
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/
> commands-index.js#82

I've started down this path, so I'll see how far I can get.
Summary: Inspect element / command buttons no longer shown for new B2G servers → GCLI actor fails to getRequisition for new B2G servers
See Also: → 1161131
/r/8107 - Bug 1160361 - Move commands-index to the server. r=bgrins
/r/8109 - Bug 1160361 - Catch GCLI's load of definitions for non-Firefox. r=bgrins
/r/8111 - Bug 1160361 - Move gclicommands.properties to toolkit. r=bgrins
/r/8113 - Bug 1160361 - Move gcli.properties to toolkit. r=bgrins
/r/8115 - Bug 1160361 - Move GCLI prefs to all.js. r=bgrins
/r/8117 - Bug 1160361 - Load tilt commands from ToolboxButtons to avoid in non-Firefox. r=bgrins
/r/8119 - Bug 1160361 - Skip Telemetry calls for non-Firefox. r=bgrins
/r/8121 - Bug 1160361 - Abort tilt commands when remote. r=bgrins

Pull down these commits:

hg pull -r 7246c8e2a0e2fb91276650f0cfa1f831e9ff01cf https://reviewboard-hg.mozilla.org/gecko/
Attachment #8601083 - Flags: review?(bgrinstead)
Comment on attachment 8601083 [details]
MozReview Request: bz://1160361/jryans

/r/8107 - Bug 1160361 - Move commands-index to the server. r=bgrins
/r/8109 - Bug 1160361 - Catch GCLI's load of definitions for non-Firefox. r=bgrins
/r/8111 - Bug 1160361 - Move gclicommands.properties to toolkit. r=bgrins
/r/8113 - Bug 1160361 - Move gcli.properties to toolkit. r=bgrins
/r/8115 - Bug 1160361 - Move GCLI prefs to all.js. r=bgrins
/r/8117 - Bug 1160361 - Load tilt commands from ToolboxButtons to avoid in non-Firefox. r=bgrins
/r/8119 - Bug 1160361 - Skip Telemetry calls for non-Firefox. r=bgrins
/r/8121 - Bug 1160361 - Abort tilt commands when remote. r=bgrins

Pull down these commits:

hg pull -r 7246c8e2a0e2fb91276650f0cfa1f831e9ff01cf https://reviewboard-hg.mozilla.org/gecko/
Attachment #8601083 - Flags: review?(jwalker)
(In reply to Gregory Szorc [:gps] from comment #6)
> Comment on attachment 8601083 [details]
> MozReview Request: bz://1160361/jryans
> 
> /r/8107 - Bug 1160361 - Move commands-index to the server. r=bgrins
> /r/8109 - Bug 1160361 - Catch GCLI's load of definitions for non-Firefox.
> r=bgrins
> /r/8111 - Bug 1160361 - Move gclicommands.properties to toolkit. r=bgrins
> /r/8113 - Bug 1160361 - Move gcli.properties to toolkit. r=bgrins
> /r/8115 - Bug 1160361 - Move GCLI prefs to all.js. r=bgrins
> /r/8117 - Bug 1160361 - Load tilt commands from ToolboxButtons to avoid in
> non-Firefox. r=bgrins
> /r/8119 - Bug 1160361 - Skip Telemetry calls for non-Firefox. r=bgrins
> /r/8121 - Bug 1160361 - Abort tilt commands when remote. r=bgrins
> 
> Pull down these commits:
> 
> hg pull -r 7246c8e2a0e2fb91276650f0cfa1f831e9ff01cf
> https://reviewboard-hg.mozilla.org/gecko/

Joe, I asked :gps to add you as a reviewer for one part to this (need to file a reviewboard bug to add this feature).  I wanted you to take a look at changes to toolkit/devtools/gcli/source/* -> is it OK to land these in m-c first and then have them migrate back to the gcli repo?
Attachment #8601083 - Flags: review?(bgrinstead) → review+
Comment on attachment 8601083 [details]
MozReview Request: bz://1160361/jryans

https://reviewboard.mozilla.org/r/8105/#review6889

The changes seem pretty straightforward.  Before landing, I'd like to make sure Joe takes a look with regard to how we land the stuff inside the gcli source (and would welcome any feedback on the implementation from him).

::: toolkit/devtools/gcli/commands/index.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Looks like this is a brand new file.  Any way you could do a copy using hg / git so that we can maintain blame history?  Same comment for all of the moved files in this commit queue.
https://reviewboard.mozilla.org/r/8105/#review6899

All looks good to me,

I'll handle syncing changes with the GCLI repo.

However, I think we need to take care when moving l10n files - translated copies of those files will need to be moved to keep up, I think. Maybe we should get feedback from :flod? Sorry to keep passing the f? !
Attachment #8601083 - Flags: review?(jwalker) → review+
(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #9)
> https://reviewboard.mozilla.org/r/8105/#review6899
> 
> All looks good to me,
> 
> I'll handle syncing changes with the GCLI repo.
> 
> However, I think we need to take care when moving l10n files - translated
> copies of those files will need to be moved to keep up, I think. Maybe we
> should get feedback from :flod? Sorry to keep passing the f? !
Flags: needinfo?(francesco.lodolo)
Flod, for more context, I need to move 2 existing properties files from browser to toolkit.

Is there are a good way to handle this to keep translated files in sync?
If I read the patch right, gcli.properties and gclicommands.properties move from browser/devtools to toolkit/global/devtools

Are there any other changes besides the move?

The only thing we can do is to send out an email to localizers explaining that they can simply move the file in Mercurial, but that doesn't help people on external tools (hopefully translation memory will make things a little easier).

Side note: my only concern would be that these 2 files are huge. We talked in the past about splitting devtools from browser for localization, also to get a more realistic picture of the completion status of localized builds, but I have the feeling the more stuff moves to toolkit, the harder this would get.
Flags: needinfo?(francesco.lodolo)
Those were the only 2 files that I spotted.
Is there some variant of

    for LOCALE in ....
    do
      hg clone https://hg.mozilla.org/l10n-central/${LOCALE}
      cd ${LOCALE}
      hg mv browser/devtools/.../gcli.properties toolkit/global/devtools/.../gcli.properties
      hg mv browser/devtools/.../gclicommands.properties toolkit/global/devtools/.../gclicommands.properties
      hg commit -m "Auto Move"
      hg push
    done

That would work?
(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #12)
> Side note: my only concern would be that these 2 files are huge. We talked
> in the past about splitting devtools from browser for localization, also to
> get a more realistic picture of the completion status of localized builds,
> but I have the feeling the more stuff moves to toolkit, the harder this
> would get.

I'm unclear why it's harder from toolkit? Why's that?
(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #13)
>     for LOCALE in ....
>       hg commit -m "Auto Move"
>       hg push
>     done
> 
> That would work?

No, for two reasons. Localizers are responsible of their repositories, and that would simply not work for locales where external tools are committing to the repo.


(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #14)
> I'm unclear why it's harder from toolkit? Why's that?

Those 2 files contain almost 500 strings. That moves 500 strings from Firefox (/browser) to any product we ship (Firefox, Firefox for Android, SeaMonkey, Thunderbird).

Example: a locale only shipping Fennec will grow by 500 strings in one commit.
(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #15)
> (In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc)
> from comment #14)
> > I'm unclear why it's harder from toolkit? Why's that?
> 
> Those 2 files contain almost 500 strings. That moves 500 strings from
> Firefox (/browser) to any product we ship (Firefox, Firefox for Android,
> SeaMonkey, Thunderbird).
> 
> Example: a locale only shipping Fennec will grow by 500 strings in one
> commit.

It is more strings for all servers, but it seems required by the current design of GCLI, since it tells the client what commands exist, including their strings.  I don't see an obvious way around this.

I'll proceed with landing (after fixing the patches to be file moves), and then notify localizers.
Attachment #8601083 - Attachment is obsolete: true
Attachment #8620209 - Flags: review+
Attachment #8620210 - Flags: review+
Attachment #8620211 - Flags: review+
Attachment #8620212 - Flags: review+
Attachment #8620213 - Flags: review+
Attachment #8620214 - Flags: review+
Attachment #8620215 - Flags: review+
Attachment #8620216 - Flags: review+
Whiteboard: [devedition-40] → [polish-backlog]
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.