Closed Bug 1208112 Opened 4 years ago Closed 4 years ago

Package DevTools client in SeaMonkey now that it's been moved out of /browser/

Categories

(SeaMonkey :: Installer, defect)

defect
Not set

Tracking

(seamonkey2.41 fixed)

RESOLVED FIXED
seamonkey2.41
Tracking Status
seamonkey2.41 --- fixed

People

(Reporter: philip.chee, Assigned: philip.chee)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

q.v. Bug 912121 - Move DevTools code to /devtools top-level directory
It may be a little early for this step still?  We still make use of some files from the /browser directory today inside /devtools/client.
Summary: Package Devtools client now that it's been moved out of /browser/ → Package DevTools client now that it's been moved out of /browser/
Attached patch Patch v1.0 Package fix. (obsolete) — Splinter Review
Attachment #8665497 - Flags: review?(iann_bugzilla)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1)
> It may be a little early for this step still?  We still make use of some
> files from the /browser directory today inside /devtools/client.
I'm just packaging it at the moment. I'm not going to hook it up to the UI yet.

> +++ b/suite/moz.build
....
> +    '/mozilla/devtools/client',
When I do this the client part goes into omni.ja!/modules/mozilla/devtools/client/ when it should go into /modules/devtools/client/
Any advice?
Flags: needinfo?(jryans)
Attachment #8665497 - Flags: review?(iann_bugzilla)
(In reply to Philip Chee from comment #3)
> > +++ b/suite/moz.build
> ....
> > +    '/mozilla/devtools/client',
> When I do this the client part goes into
> omni.ja!/modules/mozilla/devtools/client/ when it should go into
> /modules/devtools/client/
> Any advice?

Hmm.  With Firefox, the files are installed to <obj>/dist/bin/browser/modules/devtools/client.

They then become part of the browser/omni.ja!modules/devtools/client.

AIUI, this works because browser/moz.build includes:

  DIST_SUBDIR = 'browser'
  export('DIST_SUBDIR')

Do you have a similar thing?
> AIUI, this works because browser/moz.build includes:

>   DIST_SUBDIR = 'browser'
>   export('DIST_SUBDIR')

> Do you have a similar thing?
No we don't and that's the problem. Firefox has a mezzanine /browser/ level and the Firefox devtools code is spread over two omni.ja's. SeaMonkey has one unified omni.ja without the intermediate level[1].

[1] This was created so that Desktop Firefox and Metrofox could be shipped in the same package. Now that the Metro code has been removed the /browser/ is redundant and the two omni.ja's can be re-united. But that's grist for another bug really.

CC a build expert for some advice.
Flags: needinfo?(jryans)
Attachment #8666375 - Flags: review?(mh+mozilla)
Attachment #8666375 - Flags: review?(jryans)
Comment on attachment 8666375 [details] [diff] [review]
moz.build Patch for mozilla-central

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

I don't have a strong opinion here since it's guarded by MOZ_SUITE.

I'll leave it up to the build peers.
Attachment #8666375 - Flags: review?(jryans)
Summary: Package DevTools client now that it's been moved out of /browser/ → Package DevTools client in SeaMonkey now that it's been moved out of /browser/
Comment on attachment 8666375 [details] [diff] [review]
moz.build Patch for mozilla-central

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

::: toolkit/toolkit.mozbuild
@@ +151,5 @@
>      '/other-licenses/snappy',
>  ]
>  
> +if CONFIG['MOZ_SUITE']:
> +    DIRS += ['/devtools/client']

You should just add this to some suite/ moz.build.
Attachment #8666375 - Flags: review?(mh+mozilla) → review-
>> +if CONFIG['MOZ_SUITE']:
>> +    DIRS += ['/devtools/client']
> 
> You should just add this to some suite/ moz.build.
How do I do this without running into the problem in Comment 3 ?
Flags: needinfo?(mh+mozilla)
DIRS += ['/devtools/client'] somewhere in a suite/ moz.build
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #9)
> DIRS += ['/devtools/client'] somewhere in a suite/ moz.build

The error occurred while processing the following file:
    c:/t1/hg/comm-central/suite/moz.build
The underlying problem is we referenced a path that does not exist. That path is:
    c:/t1/hg/comm-central/devtools/client/moz.build
Either create the file if it needs to exist or do not reference it.

Got a better suggestion?
(In reply to Mike Hommey [:glandium] from comment #9)
> DIRS += ['/devtools/client'] somewhere in a suite/ moz.build

The error occurred while processing the following file:
    c:/t1/hg/comm-central/suite/moz.build
The underlying problem is we referenced a path that does not exist. That path is:
    c:/t1/hg/comm-central/devtools/client/moz.build
Either create the file if it needs to exist or do not reference it.

Got a better suggestion?
Flags: needinfo?(mh+mozilla)
Putting it in ** app.mozbuild ** seems to work
Flags: needinfo?(mh+mozilla)
This packages the Devtools Client which has been moved out of /browser/ into a new top level directory. It isn't usable at the moment since the L10n files are still in /browser/
Attachment #8665497 - Attachment is obsolete: true
Attachment #8666375 - Attachment is obsolete: true
Attachment #8669343 - Flags: review?(iann_bugzilla)
Attachment #8669343 - Attachment is obsolete: true
Attachment #8669343 - Flags: review?(iann_bugzilla)
Attachment #8675296 - Flags: review?(iann_bugzilla)
Bug 1203159 has changed this up a bit.

You would now do something like:

app.mozbuild:

DIRS += ['/devtools']

confvars.sh:

MOZ_DEVTOOLS=all
Comment on attachment 8675296 [details] [diff] [review]
Patch v2.1 Include the devtools preferences file

Need to fix bitrot
Attachment #8675296 - Flags: review?(iann_bugzilla)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #15)
> Bug 1203159 has changed this up a bit.
> 
> You would now do something like:
> 
> app.mozbuild:
> 
> DIRS += ['/devtools']
> 
> confvars.sh:
> 
> MOZ_DEVTOOLS=all

Thanks!
Attachment #8675296 - Attachment is obsolete: true
Attachment #8678226 - Flags: review?(iann_bugzilla)
Unfortunately things changed slightly again in bug 1217687, where we tried to simplify this a bit for all apps.  This has only landed to fx-team so far, should merge to m-c soon.

/toolkit now does the DIRS += ['/devtools'] step, so you no longer want this in your app.mozbuild (assuming in includes toolkit/toolkit.mozbuild already).

So, I would guess remove that part, but otherwise it should be good.
Attached patch Patch v4Splinter Review
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #18)
> Unfortunately things changed slightly again in bug 1217687, where we tried
> to simplify this a bit for all apps.  This has only landed to fx-team so
> far, should merge to m-c soon.
> 
> /toolkit now does the DIRS += ['/devtools'] step, so you no longer want this
> in your app.mozbuild (assuming in includes toolkit/toolkit.mozbuild already).
> 
> So, I would guess remove that part, but otherwise it should be good.
Done.
Attachment #8678226 - Attachment is obsolete: true
Attachment #8678226 - Flags: review?(iann_bugzilla)
Attachment #8678568 - Flags: review?(neil)
Attachment #8678568 - Flags: review?(iann_bugzilla)
Blocks: 1218145
Attachment #8678568 - Flags: review?(iann_bugzilla) → review+
http://hg.mozilla.org/comm-central/rev/ba4137b57c77
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.41
Comment on attachment 8678568 [details] [diff] [review]
Patch v4

A followup patch is needed. Package the devtools.js preference file only if the Devtools client is built and packaged. The devtools manifest should be packaged for all values of MOZ_DEVTOOLS


 ; DevTools
 @RESPATH@/chrome/devtools@JAREXT@
 @RESPATH@/chrome/devtools.manifest
+#if MOZ_DEVTOOLS == all
 @RESPATH@/@PREF_DIR@/devtools.js
+#endif
Attachment #8678568 - Flags: review?(neil)
Blocks: 1223344
Blocks: smdevtools
You need to log in before you can comment on or make changes to this bug.