Closed Bug 298431 Opened 20 years ago Closed 19 years ago

Rewrite xforms build system so it works with XULRunner

Categories

(Core Graveyard :: XForms, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: allan, Assigned: allan)

References

()

Details

(Keywords: fixed1.8)

Attachments

(5 files, 5 obsolete files)

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>.
Attached patch First try (obsolete) (deleted) — Splinter Review
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.
btw, I haven't actually tested it with XULRunner yet. I'm trying to figure out how extensions actually work for XULRunner.. currently building...
(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.
(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 :(
I'll just continue the discussion with myself here :-) ... XULRunner possibly needs to be an application in install.rdf? Would make sense.
(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. :(
(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: toolkit@mozilla.org
*** Bug 286202 has been marked as a duplicate of this bug. ***
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.
(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?
(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?
Attached patch Better try (obsolete) (deleted) — Splinter Review
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.
(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.
(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
(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.
Attached patch Patch (obsolete) (deleted) — Splinter Review
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-
Attachment #188929 - Flags: review?(doronr)
(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?
(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?
Attachment #189030 - Flags: approval1.8b4? → approval1.8b4+
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.
Attached patch Tinderbox patch (deleted) — Splinter Review
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 on attachment 198290 [details] [diff] [review] Tinderbox patch coop will land.
Attachment #198290 - Flags: review?(chase) → review+
(In reply to comment #29) > (From update of attachment 198290 [details] [diff] [review] [edit]) > coop will land. > Landed.
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?
No longer depends on: toolkit@mozilla.org
Whiteboard: xf-to-branch
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.
Attached patch Bustage fix (obsolete) (deleted) — Splinter Review
Definately not a beauty, but it should fix the distclean problem
Attached patch Bustage fix, for real (deleted) — Splinter Review
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+
(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...
(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.
Attached patch Use shell to test DIRS before looping (obsolete) (deleted) — Splinter Review
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...
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 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?
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?
(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.
(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...
(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.
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-
(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.
(In reply to comment #48) > Having "distclean:: LOOP_OVER_DIRS=" instead of "distclean:: DIRS=" resolves the > issue. Allan - are you comfortable with this fix?
(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.
Attached patch Use LOOP_OVER_DIRS= instead (deleted) — Splinter Review
Attachment #200344 - Flags: review?(benjamin)
Attachment #200344 - Flags: review?(benjamin) → review+
Attachment #200079 - Attachment is obsolete: true
I'm really having fun with this.... :( extensions/xforms/package needs to be killed from allmakefiles.sh too.
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+
Attachment #200344 - Flags: approval1.8rc1?
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+
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
Checked in on 1.8 branch
Status: ASSIGNED → RESOLVED
Closed: 19 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.

Attachment

General

Creator:
Created:
Updated:
Size: