The default bug view has changed. See this FAQ.

Rewrite xforms build system so it works with XULRunner

RESOLVED FIXED

Status

Core Graveyard
XForms
--
enhancement
RESOLVED FIXED
12 years ago
8 months ago

People

(Reporter: Allan Beaufour, Assigned: Allan Beaufour)

Tracking

({fixed1.8})

Trunk
fixed1.8

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 5 obsolete attachments)

(Assignee)

Description

12 years ago
To make xforms work with XULRunner and just generally work better, we should
implement the same techniques as used by the DOM Inspector, ie. XPI_NAME.

* extensions/inspector/Makefile.in
* extensions/inspector/jar.mn
XPI_NAME ships files to dist/xpi-stage/<XPI_NAME> instead of to dist/bin, which
helps keep the extension build separate from the main app. It can also be used
in conjunction with a couple other vars you find in
extensions/inspector/Makefile.in to build a chrome.manifest from a jar.mn file
and to automatically package that xpi-stage directory into a XPI file and/or
ship it to dist/bin/extensions/<extension-ID>.
(Assignee)

Comment 2

12 years ago
Created attachment 188296 [details] [diff] [review]
First try

Ok, this patch:
* adds the magic to Makefile.in
* moves package/install.rdf to root dir.
* kills package/

This means that the extension (and the XPI) is compiled to dist/bin/xpi-stage/.


Problems:
* building the xpi for suite not working
  But actually we should drop suite shouldn't we?

* schemavalidation is not part of the xpi
  But has this not always been a bit weird? We could create a
schemavalidation.xpi? Having to install two xpi's are not fantastic, but that's
the most generic approach.
(Assignee)

Comment 3

12 years ago
btw, I haven't actually tested it with XULRunner yet. I'm trying to figure out
how extensions actually work for XULRunner.. currently building...
(Assignee)

Comment 4

12 years ago
(In reply to comment #3)
> btw, I haven't actually tested it with XULRunner yet. I'm trying to figure out
> how extensions actually work for XULRunner.. currently building...

It works with XULRunner.
(Assignee)

Comment 5

12 years ago
(In reply to comment #4)
> (In reply to comment #3)
> > btw, I haven't actually tested it with XULRunner yet. I'm trying to figure out
> > how extensions actually work for XULRunner.. currently building...
> 
> It works with XULRunner.

Ok. I might have said that a little bit too early.

'make' in extensions/xforms works, and also builds an xpi. But I cannot seem to
figure out how to install the XPI. unzipping under
~/.mozillatest/mybrowser/<profile>/extensions/{cf2812dc-6a7c-4402-b639-4d277dac4c36}
or dist/bin/extensions/{cf2812dc-6a7c-4402-b639-4d277dac4c36} does not work :(
(Assignee)

Comment 6

12 years ago
I'll just continue the discussion with myself here :-) ... XULRunner possibly
needs to be an application in install.rdf? Would make sense.
(Assignee)

Comment 7

12 years ago
(In reply to comment #6)
> I'll just continue the discussion with myself here :-) ... XULRunner possibly
> needs to be an application in install.rdf? Would make sense.

from irc:
<bsmedberg> beaufour: you need what we don't have yet... an em:targetApplication
marker for "the toolkit" in general.

:(
(Assignee)

Updated

12 years ago
Depends on: 299716

Comment 8

12 years ago
(In reply to comment #2)
> Created an attachment (id=188296) [edit]
> First try
> 
> Ok, this patch:
> * adds the magic to Makefile.in
> * moves package/install.rdf to root dir.
> * kills package/
> 
> This means that the extension (and the XPI) is compiled to dist/bin/xpi-stage/.
> 
> 
> Problems:
> * building the xpi for suite not working
>   But actually we should drop suite shouldn't we?
> 
> * schemavalidation is not part of the xpi
>   But has this not always been a bit weird? We could create a
> schemavalidation.xpi? Having to install two xpi's are not fantastic, but that's
> the most generic approach.

For suite, all we need to do is package install.js and the 2 contents.rdf files
- I believe that is possible.

We really should have just one XPI.  Perhaps change the schema validation build
system to check if XForms is built and push the files to the stage dir?  Not
that I know much about the new build system
No longer depends on: 299716
(Assignee)

Comment 9

12 years ago
*** Bug 286202 has been marked as a duplicate of this bug. ***

Comment 10

12 years ago
I'd like to see suite support continue as long as it is just a pakaging/build
issue.  I could see dropping support for it if it starts to interfere with code
logic, requires supporting code paths or ifdefs just for it, etc.  It still has
faithful users.

I'd like to keep it all one .xpi as long as we can.  Seperate xpi's lead to
versioning and compatibility issues down the road, plus support and test issues.
(Assignee)

Comment 11

12 years ago
(In reply to comment #10)
> I'd like to see suite support continue as long as it is just a pakaging/build
> issue.  I could see dropping support for it if it starts to interfere with code
> logic, requires supporting code paths or ifdefs just for it, etc.  It still has
> faithful users.

I agree. But already, our preferences for whitelists does not work for suite...
nevertheless, I'll see what is needed for suite. It's probably just a
configuration issue.

> I'd like to keep it all one .xpi as long as we can.  Seperate xpi's lead to
> versioning and compatibility issues down the road, plus support and test issues.

Where did I say that I wanted seperate XPIs? I didn't mean that.

Doron: Why did you drop the dep. for bug 299716?
Depends on: 299716
(Assignee)

Comment 12

12 years ago
(In reply to comment #11)
> Where did I say that I wanted seperate XPIs? I didn't mean that.

My brain's still on vacation :(... seperate XPIs for schemavalidation and
xforms, of course, yes. I kept reading "seperate XPIs for suite and firefox"...
sorry for that.

As for a seperate XPI for schemavalidation: What if somebody else wants to use
schemavalidation, and already has it installed? Or just want to install that,
and not xforms?
(Assignee)

Comment 13

12 years ago
Created attachment 188415 [details] [diff] [review]
Better try

Mr. cut'n'paste does it again... this one should work for suite, including
install.js. I've also "templatized" install.rdf so versions, uuid, etc. only
needs to be changed in Makefile.in.

There's still the "schemavalidation.xpi" problem though.
Sorry about removing the dependency, not sure how that happened.

http://lxr.mozilla.org/seamonkey/source/calendar/lightning/Makefile.in#51

Using DIRS seems to do what we want, to get the schema validation files.

Comment 15

12 years ago
(In reply to comment #12)
> (In reply to comment #11)
> > Where did I say that I wanted seperate XPIs? I didn't mean that.
> 
> My brain's still on vacation :(... seperate XPIs for schemavalidation and
> xforms, of course, yes. I kept reading "seperate XPIs for suite and firefox"...
> sorry for that.
> 
> As for a seperate XPI for schemavalidation: What if somebody else wants to use
> schemavalidation, and already has it installed? Or just want to install that,
> and not xforms?


Yeah, that is a tough nut to crack.  I personally think that if anyone else
wants to use schema-validation, then we should probably see about getting it
into the core.  I think that it'll be more widely used than just XForms in the
future.  W3C specs to come will probably require it.

Can an extension put a dependency on another extension and prompt the user to
get it if it isn't there (but not if it is)?  Maybe somehow indicate the need in
the Extensions dialog?  Also if we decide to do that, we'll have to get the
nightly builds to build it as an xpi and put it in the xpi dir.
(Assignee)

Comment 16

12 years ago
(In reply to comment #14)
> Using DIRS seems to do what we want, to get the schema validation files.

As far as I can see it just adds schema-validation directory to the build list.
I think EXTRA_COMPONENTS is our friend for packaging.
Status: NEW → ASSIGNED
(Assignee)

Comment 17

12 years ago
(In reply to comment #15)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Where did I say that I wanted seperate XPIs? I didn't mean that.
> > 
> > My brain's still on vacation :(... seperate XPIs for schemavalidation and
> > xforms, of course, yes. I kept reading "seperate XPIs for suite and firefox"...
> > sorry for that.
> > 
> > As for a seperate XPI for schemavalidation: What if somebody else wants to use
> > schemavalidation, and already has it installed? Or just want to install that,
> > and not xforms?
> 
> 
> Yeah, that is a tough nut to crack.  I personally think that if anyone else
> wants to use schema-validation, then we should probably see about getting it
> into the core.  I think that it'll be more widely used than just XForms in the
> future.  W3C specs to come will probably require it.

Ok. Until somebody complains about it, let's keep packaging together with
xforms.xpi.
(Assignee)

Comment 18

12 years ago
Created attachment 188929 [details] [diff] [review]
Patch

This patch builds and packages schema-validation too. It _should_ work on all
platforms, but it needs testing.

It has one bug, which comes from the packaging of schema-validation: When
schema-validation is built it is installed in dist/bin/components. This is not
necessary as it is packaged with xforms, which is installed in
dist/extensions/{cf...}. So it actually ends up being installed twice, but I do
not think it should create problems for us?

I haven't given it much thought, but instead of using DIRS we could possibly
call 'make -f extensions/schema-validation/Makefile NO_DIST_INSTALL=1' to build
schemavalidation and then package the files from extensions/schema-validation
instead? (and include correct magic for creation of Makefiles there)
Attachment #188296 - Attachment is obsolete: true
Attachment #188415 - Attachment is obsolete: true
Attachment #188929 - Flags: review?(doronr)
cc: pedemonte to do mac testing :)


Comment on attachment 188929 [details] [diff] [review]
Patch

>diff -X patch-excludes -uprN -U8 xforms/install.rdf xforms.xpi/install.rdf
>--- xforms/install.rdf	1970-01-01 01:00:00.000000000 +0100
>+++ xforms.xpi/install.rdf	2005-07-06 11:02:30.000000000 +0200
>@@ -0,0 +1,41 @@
>+<?xml version="1.0"?>
>+
>+#filter substitution
>+
>+<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>+     xmlns:em="http://www.mozilla.org/2004/em-rdf#">
>+
>+  <Description about="urn:mozilla:install-manifest">
>+    <em:id>@ID@</em:id>
>+    <em:version>@PACKAGE_VERSION@</em:version>
>+
>+    <em:targetApplication>
>+      <!-- Firefox -->
>+      <Description>
>+        <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
>+        <em:minVersion>@FIREFOX_VERSION@</em:minVersion>
>+        <em:maxVersion>@FIREFOX_VERSION@</em:maxVersion>
>+      </Description>
>+    </em:targetApplication>
>+
>+    <em:targetApplication>
>+    <!-- Mozilla -->
>+      <Description>
>+        <em:id>{86c18b42-e466-45a9-ae7a-9b95ba6f5640}</em:id>
>+        <em:minVersion>@MOZILLA_VERSION@</em:minVersion>
>+        <em:maxVersion>@MOZILLA_VERSION@</em:maxVersion>
>+      </Description>
>+    </em:targetApplication>
>+
>+    <em:name>Mozilla XForms</em:name>
>+    <em:description>XForms support for Mozilla</em:description>
>+    <em:homepageURL>http://www.mozilla.org/projects/xforms/</em:homepageURL>
>+    <em:file>
>+      <Description about="urn:mozilla:extension:file:xforms.jar">
>+        <em:package>content/xforms/</em:package>
>+        <em:locale>locale/en-US/xforms/</em:locale>
>+      </Description>
>+    </em:file>
>+  </Description>
>+</RDF>
>+

Hmm, what about en-US, should that perhaps be configurable?  So that someone
could build a say japanese locale into the xpi?  Probably don't have to worry
about it now.
Attachment #188929 - Flags: superreview?(benjamin)
bsmedberg points out on irc that em:file is no longer needed since we now use
chrome.manifest, and we don't need ff 1.0 compat.
Comment on attachment 188929 [details] [diff] [review]
Patch

The schemavalidation stuff looks wrong to me: you should probably
"export XPI_NAME = xforms"
and avoid doing the separate EXTRA_COMPONENTS bits that install it from
dist/bin.

It seems to me that your install.jst needs a registerChrome instruction for
en-US.

As doron said, the install.rdf does not need (should not have) the em:file
bits.
Attachment #188929 - Flags: superreview?(benjamin) → superreview-
(Assignee)

Updated

12 years ago
Attachment #188929 - Flags: review?(doronr)
(Assignee)

Comment 23

12 years ago
Created attachment 189030 [details] [diff] [review]
Patch w/Benjamin's comments addressed

(In reply to comment #22)
> (From update of attachment 188929 [details] [diff] [review] [edit])
> The schemavalidation stuff looks wrong to me:

I didn't like it much either.

> you should probably "export XPI_NAME = xforms"

Ah, isn't "export" a lovely little word :-) I obviously missed the point of it
in the link Doron sent.

Fixed that, killed em:file, and included a registerChrome for the locale in
install.js.
Attachment #188929 - Attachment is obsolete: true
Attachment #189030 - Flags: superreview?(benjamin)
Attachment #189030 - Flags: review?(benjamin)
Comment on attachment 189030 [details] [diff] [review]
Patch w/Benjamin's comments addressed

I'm not sure what the deal is with approvals and xforms, but if this needs
approval it will have it after b3 gets released.
Attachment #189030 - Flags: superreview?(benjamin)
Attachment #189030 - Flags: review?(benjamin)
Attachment #189030 - Flags: review+
Attachment #189030 - Flags: approval1.8b4?
(Assignee)

Comment 25

12 years ago
(In reply to comment #24)
> (From update of attachment 189030 [details] [diff] [review] [edit])
> I'm not sure what the deal is with approvals and xforms, but if this needs
> approval it will have it after b3 gets released.

Normally we do not need approval, but we need to get the procedure for the
nightly building of the XPIs changed to handle the new build procedure. Is that
you Chase?

Updated

12 years ago
Attachment #189030 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 26

12 years ago
The change in the build system is that instead of the xforms.xpi being created
in extensions/xforms/stage it now ends up in dist/xpi-stage.
(Assignee)

Comment 27

12 years ago
This needs a patch:
http://lxr.mozilla.org/mozilla/source/tools/tinderbox/post-mozilla-rel.pl#330
(Assignee)

Comment 28

12 years ago
Created attachment 198290 [details] [diff] [review]
Tinderbox patch

Here's a patch to post-mozilla-rel.pl, changes:
* schemavalidation is built automatically through the xforms Makefile
* xpi-file is now stored in dist/xpi-stage
Attachment #198290 - Flags: review?(chase)

Comment 29

12 years ago
Comment on attachment 198290 [details] [diff] [review]
Tinderbox patch

coop will land.
Attachment #198290 - Flags: review?(chase) → review+

Comment 30

12 years ago
(In reply to comment #29)
> (From update of attachment 198290 [details] [diff] [review] [edit])
> coop will land.
> 

Landed.
(Assignee)

Comment 31

12 years ago
Comment on attachment 189030 [details] [diff] [review]
Patch w/Benjamin's comments addressed

Checked in to trunk.

It has an oooold approval for 1.8b4, which was waiting for the tinderbox patch
which went in yesterday. Re-requesting approval for rc1.
Attachment #189030 - Flags: approval1.8rc1?
(Assignee)

Updated

12 years ago
No longer depends on: 299716
(Assignee)

Updated

12 years ago
Whiteboard: xf-to-branch
(Assignee)

Comment 32

12 years ago
Argh. This has a bad influence on make distclean :(

As extensions/xforms now builds extensions/schema-validation, a distclean in
xforms also cleans schema-validation, which makes a global 'make distclean' fail
because the Makefile is cleaned from schema-validation by xforms.
(Assignee)

Comment 33

12 years ago
Created attachment 200060 [details] [diff] [review]
Bustage fix

Definately not a beauty, but it should fix the distclean problem

Comment 34

12 years ago
Created attachment 200061 [details] [diff] [review]
Bustage fix, for real

beaufour didn't get the right diff, but here's what he checked in with my
blessing, provided he won't rest (much) until he's found a better solution.
Attachment #200060 - Attachment is obsolete: true
Attachment #200061 - Flags: superreview+
Attachment #200061 - Flags: review+
(Assignee)

Comment 35

12 years ago
(In reply to comment #34)
> Created an attachment (id=200061) [edit]
> Bustage fix, for real
> 
> beaufour didn't get the right diff

Definately not my day...
(Assignee)

Comment 36

12 years ago
(In reply to comment #35)
> (In reply to comment #34)
> > Created an attachment (id=200061) [edit] [edit]
> > Bustage fix, for real
> > 
> > beaufour didn't get the right diff
> 
> Definately not my day...

AIX on SeaMonkey-Ports keeps hating my guts...:
gmake[3]: Entering directory
`/home/tbox/sb/tinderbox/AIX_5.1_Clobber/mozilla/obj-tinder/extensions/xforms'
/bin/sh: syntax error at line 1 : `;' unexpected
My guess is that it is this line:
http://lxr.mozilla.org/seamonkey/source/config/rules.mk#323
the "DIRS=" hack does not undefine DIRS on AIX, expanding LOOP_OVER_DIRS.

Anybody got an idea here?
That's because the ifdef is applied at parse-time, while the DIRS= variable
override is applied at rule execution time. Hack forthwith.
Created attachment 200079 [details] [diff] [review]
Use shell to test DIRS before looping

I don't have an old shell that doesn't like "for x in ;" to test this with, but
it should work.
Attachment #200079 - Flags: review?(allan)
Please move the distclean rule back down below rules.mk: it won't do to have
"make -C extensions/xforms" doing distclean by default...

Comment 40

12 years ago
bsmedberg: that'd be my fault. The GNU make manual said that double-colon rules
apply in order found, so I figured maybe the problem was that DIRS didn't get
cleared in time.

Anyway, what I'd much rather see is that schema-validation doesn't get built
from xforms so we can do away with this hackery. I believe that through
configure and client.mk you can make sure schema-validation gets built before
xforms gets built. That should at least limit the need for hackery to just the
packaging step, though I'd like to see a clean(er) solution for that too.
It is not sufficient to simply build schema-validation, you have to build it with

export XPI_NAME = xforms

So that the results end up as part of the xforms XPI.

Comment 42

12 years ago
Comment on attachment 189030 [details] [diff] [review]
Patch w/Benjamin's comments addressed

please re-request approval for this not part of the build change after you've
got everything working and verified.
Attachment #189030 - Flags: approval1.8rc1?

Comment 43

12 years ago
There must be some other way to get those files into the xforms XPI. I remember
there being a way to merge multiple XPIs into one, could we use that?
(Assignee)

Comment 44

12 years ago
(In reply to comment #38)
> Created an attachment (id=200079) [edit]
> Use shell to test DIRS before looping
> 
> I don't have an old shell that doesn't like "for x in ;" to test this with, but
> it should work.

I do not think it does, it's only one line in the shell and I think a (stupid)
shell will evaluate it all before doing any thing, which means that "for x in ;"
will still generate an error.

I'm even less happy about the approach, but I could just do
distclean:: LOOP_OVER_DIRS=
instead... that would work.
(Assignee)

Comment 45

12 years ago
(In reply to comment #44)
> I do not think it does, it's only one line in the shell and I think a (stupid)
> shell will evaluate it all before doing any thing,

Hmmm, forget the "stupid" part...I guess it would have to pretty darn smart to
skip sections with syntax errors in fact if the test fails...
(Assignee)

Comment 46

12 years ago
(In reply to comment #39)
> Please move the distclean rule back down below rules.mk: it won't do to have
> "make -C extensions/xforms" doing distclean by default...

It won't be, it's a prerequisite as it has no commands.
(Assignee)

Comment 47

12 years ago
Comment on attachment 200079 [details] [diff] [review]
Use shell to test DIRS before looping

(In reply to comment #44)
> (In reply to comment #38)
> > Created an attachment (id=200079) [edit] [edit]
> > Use shell to test DIRS before looping
> > 
> > I don't have an old shell that doesn't like "for x in ;" to test this with, but
> > it should work.
> 
> I do not think it does, it's only one line in the shell and I think a (stupid)
> shell will evaluate it all before doing any thing, which means that "for x in ;"
> will still generate an error.

that would be r-..
Attachment #200079 - Flags: review?(allan) → review-

Comment 48

12 years ago
(In reply to comment #44)
> (In reply to comment #38)
> > Created an attachment (id=200079) [edit] [edit]
> > Use shell to test DIRS before looping
> > 
> > I don't have an old shell that doesn't like "for x in ;" to test this with, but
> > it should work.
> 
> I do not think it does, it's only one line in the shell and I think a (stupid)
> shell will evaluate it all before doing any thing, which means that "for x in ;"
> will still generate an error.
> 
> I'm even less happy about the approach, but I could just do
> distclean:: LOOP_OVER_DIRS=
> instead... that would work.


Having "distclean:: LOOP_OVER_DIRS=" instead of "distclean:: DIRS=" resolves the
issue.

Comment 49

12 years ago
(In reply to comment #48)
> Having "distclean:: LOOP_OVER_DIRS=" instead of "distclean:: DIRS=" resolves the
> issue.

Allan - are you comfortable with this fix?
(Assignee)

Comment 50

12 years ago
(In reply to comment #49)
> (In reply to comment #48)
> > Having "distclean:: LOOP_OVER_DIRS=" instead of "distclean:: DIRS=" resolves the
> > issue.
> 
> Allan - are you comfortable with this fix?

I have already started walking down the hacky path... why turn around? :)

It's ok I guess, though I haven't looked through "make distclean" to see if
LOOP_OVER_DIRS is used for something else.
(Assignee)

Comment 51

12 years ago
Created attachment 200344 [details] [diff] [review]
Use LOOP_OVER_DIRS= instead
Attachment #200344 - Flags: review?(benjamin)
Attachment #200344 - Flags: review?(benjamin) → review+
(Assignee)

Updated

12 years ago
Attachment #200079 - Attachment is obsolete: true
(Assignee)

Comment 52

12 years ago
Created attachment 200347 [details] [diff] [review]
remove extensions/xforms/package

I'm really having fun with this.... :( extensions/xforms/package needs to be
killed from allmakefiles.sh too.
(Assignee)

Updated

12 years ago
Attachment #200347 - Flags: review?(benjamin)
Comment on attachment 200347 [details] [diff] [review]
remove extensions/xforms/package

Extra/missing files don't really hurt anyone (it's just an annoying warning at
configure-time).
Attachment #200347 - Flags: review?(benjamin)
Attachment #200347 - Flags: review+
Attachment #200347 - Flags: approval1.8rc1+

Updated

12 years ago
Attachment #200344 - Flags: approval1.8rc1?

Comment 54

12 years ago
Attachment 200344 [details] [diff] checked in to trunk.

Checking in Makefile.in;
/cvsroot/mozilla/extensions/xforms/Makefile.in,v  <--  Makefile.in
new revision: 1.43; previous revision: 1.42
done
Attachment #200344 - Flags: approval1.8rc1? → approval1.8rc1+
(Assignee)

Comment 55

12 years ago
Attachment 200347 [details] [diff] checked in to trunk.

Checking in allmakefiles.sh;
/cvsroot/mozilla/allmakefiles.sh,v  <--  allmakefiles.sh
new revision: 1.588; previous revision: 1.587
done
(Assignee)

Comment 56

12 years ago
Checked in on 1.8 branch
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.