Closed Bug 1317674 Opened 3 years ago Closed 3 years ago

Port |Bug 1316450 - Enforce that nothing new depends on the XPCOM glue| to TB etc.

Categories

(Thunderbird :: Build Config, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 53.0

People

(Reporter: jorgk, Assigned: aleth)

References

Details

Attachments

(1 file, 11 obsolete files)

3.96 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
Bustage due to bug 1316450:

mozbuild.frontend.reader.SandboxValidationError: 
==============================
ERROR PROCESSING MOZBUILD FILE
==============================

The error occurred while processing the following file or one of the files it includes:
c:/builds/moz2_slave/tb-c-cen-w64-ntly-000000000000/build/calendar/base/backend/libical/build/moz.build

The error occurred when validating the result of the execution. The reported error is:
calbasecomps depends on the XPCOM glue. No new dependency on the XPCOM glue is allowed.

First seen Daily build here Tue Nov 15, 11:17:54
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=f88a5eda49535c043f7b61cb11582b2a7c920b9f
and then here Tue Nov 15, 12:02:48
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=e8c2d64742f4c6a33ae4463e0b6436dc37edabcb
Blocks: 1316450
Component: General → Build Config
Flags: needinfo?(philipp)
Product: Thunderbird → Calendar
If it's just libical, would it make sense to bite the bullet now and switch to ical.js?
...it's not just libical. There's also (independently) one of the perennial split build system path issues for 'thunderbird' itself, for which:
obj.relativedir = mail/app
MOZ_BUILD_APP = ../mail/app
and so the check in emitter.py fails.
Product: Calendar → Thunderbird
(In reply to aleth [:aleth] from comment #2)
> If it's just libical, would it make sense to bite the bullet now and switch
> to ical.js?
No. This has been discussing in the regular Tuesday meetings for a long time now. JS is apparently not ready. On top of some minor problems, like the notorious bug 1294668, there are apparently performance issues. MakeMyDay always talks about "rewriting the parser". I started using IcalJS in Daily until I got hit by some of the notorious problems, events not showing due to that, so I had to switch back to libical for my day-to-day operation.

Sticking libical into C-C core has also been discussed, but as far as I see no action was taken. Miraculously bug 1314955 didn't get landed on Gecko52 despite r+. That bug would have broken the binary add-on completely.
(In reply to Jorg K (GMT+1) from comment #4)
> Sticking libical into C-C core has also been discussed, but as far as I see
> no action was taken. Miraculously bug 1314955 didn't get landed on Gecko52
> despite r+. That bug would have broken the binary add-on completely.

As you say, libical is probably going to be broken in a matter of days anyway, therefore some decision from Calendar on how to proceed is required now anyway.
This is a somewhat hacky check for whether it's a split build system build or not, but I can't think of any scenario where a m-c build would have such a relative path in MOZ_BUILD_APP, so it seems the easiest solution.
Attachment #8810821 - Flags: review?(mh+mozilla)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Aleth, is this meant to fix the build failure described in comment #0? I still see it in a local build. I will hack this now to get some daily builds going on try.
Flags: needinfo?(aleth)
(In reply to Jorg K (GMT+1) from comment #7)
> Aleth, is this meant to fix the build failure described in comment #0? I
> still see it in a local build. I will hack this now to get some daily builds
> going on try.

No, the patch is for comment 3 - you can see that failure locally when you disable building calendar (comment 0).
Flags: needinfo?(aleth)
Here's a (temporary while libical is with us) patch to avoid the build bustage from comment 0.
Attachment #8810904 - Flags: review?(mh+mozilla)
> The error occurred when validating the result of the execution. The reported error is:
>    suite depends on the XPCOM glue. No new dependency on the XPCOM glue is allowed.

Thanks for trying to fix this. SeaMonkey still breaks with the patch. Does im build?
(In reply to Frank-Rainer Grahl from comment #11)
> > The error occurred when validating the result of the execution. The reported error is:
> >    suite depends on the XPCOM glue. No new dependency on the XPCOM glue is allowed.
> 
> Thanks for trying to fix this. SeaMonkey still breaks with the patch. Does
> im build?

Instantbird would be fine with this patch if it were not for purplexpcom.
This would be very helpful to temporarily remove the bustage from Instantbird (temporary because the way purplexpcom is built will have to change once binary components are no more).
Attachment #8810999 - Flags: review?(mh+mozilla)
I took the liberty to roll these into one. (There was also a problem with a comma at the end of the line.) Apparently SeaMonkey still has a problem building.
Attachment #8810821 - Attachment is obsolete: true
Attachment #8810904 - Attachment is obsolete: true
Attachment #8810999 - Attachment is obsolete: true
Attachment #8810821 - Flags: review?(mh+mozilla)
Attachment #8810904 - Flags: review?(mh+mozilla)
Attachment #8810999 - Flags: review?(mh+mozilla)
Attachment #8811042 - Flags: review?(mh+mozilla)
Sorry for rolling these into one without Aleth's agreement. If this patch can't be approved, then please consider the predecessor in attachment 8810821 [details] [diff] [review].
Attachment #8811042 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd867e3258be1a56c712f6dfa9f8b8532f7a8ff
Bug 1317674 - Use the correct path to the binary for comm builds in the XPCOM glue check + exceptions for Calendar and Instantbird. r=glandium
This shouldn't be too hard to fix. Looks like this is coming from
+ moz_build_app = substs.get('MOZ_BUILD_APP')
+ if moz_build_app.startswith('../'):
and moz_build_app comes out null or undefined. Sorry, not a python expert at all.
In JS we would just go
  moz_build_app = substs.get('MOZ_BUILD_APP') || ""

;-)

Funny thought that before it didn't complain about:
- (substs.get('MOZ_APP_NAME'), '%s/app' % substs.get('MOZ_BUILD_APP')),
if substs.get('MOZ_BUILD_APP') was indeed undefined.
(In reply to Jorg K (GMT+1) from comment #19)
> How about:
> if moz_build_app is not None and moz_build_app.startswith('../'):
> ?

I don't think that's the right fix. If moz_build_app is None adding the exceptions makes no sense in the first place. I'll try to understand what's going on here later.
Agreed, but that's a pre-existing problem that we shouldn't try to fix here. As the backout shows, it even occurs in an M-C-only environment.
This basically worked:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=955d0f2291eea74e97c90da9b13d751354c873b3&selectedJob=25610

Somehow the build still came out in orange, but the "startswith" error is gone. Now we see:
State Changed: unlock buildroot
program finished with exit code 2
elapsedTime=562.579136
TinderboxPrint: check<br/>6294/<em class="testfail">1</em>
Unknown Error: command finished with exit code: 2

It would still be good to land this and follow up any remaining problems elsewhere.
Attachment #8811211 - Flags: review?(mh+mozilla)
I have another try run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=beec811dd591ca836bf356b97442f3fc687ae93c

This time the error is a bit more conclusive:

TEST-UNEXPECTED-FAIL | /builds/slave/tb-try-c-cen-m64-d-00000000000/build/mozilla/python/mozbuild/mozbuild/test/frontend/test_emitter.py | TestEmitterBasic.test_allowed_xpcom_glue, line 1193: Lists differ: [(u'purplexpcom', u'extensions... != []
OK, let's see whether we can get rid of the error mentioned in comment #24 like this.

New try run on C-C try here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=21dd9e8a812e50e4f4efccd63e20149030add55a
Attachment #8811211 - Attachment is obsolete: true
Attachment #8811211 - Flags: review?(mh+mozilla)
Attachment #8811324 - Flags: review?(mh+mozilla)
This fixes the tests on try. (Sorry jorgk for the overlap with your work, which I saw too late.)
Attachment #8811347 - Flags: review?(mh+mozilla)
Attachment #8811042 - Attachment is obsolete: true
Flags: needinfo?(aleth)
Comment on attachment 8811324 [details] [diff] [review]
Slight change to fix "startswith" error and another issue (v2).

Aleth'es looks better ;-)
Attachment #8811324 - Attachment is obsolete: true
Attachment #8811324 - Flags: review?(mh+mozilla)
Refreshed.
Attachment #8811347 - Attachment is obsolete: true
Attachment #8811347 - Flags: review?(mh+mozilla)
Attachment #8811524 - Flags: review?(mh+mozilla)
With this patch, Thunderbird builds fine:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a6e9cea2087441d4d4e2d9357ceafeaa000a7b30

Please approve asap.
Comment on attachment 8811524 [details] [diff] [review]
Use the correct path to the binary for comm builds in the XPCOM glue check + exceptions for Calendar and Instantbird

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

::: python/mozbuild/mozbuild/test/frontend/test_emitter.py
@@ +1180,5 @@
>  
>          for path, names in allowed.iteritems():
> +            if (path.startswith('mailnews/') or
> +                path.startswith('calendar/') or
> +                path.startswith('extensions/')):

path.starswith(('mailnews/', 'calendar/', 'extensions/'))

Please be more specific for extensions/ because that adds mozilla-central's extensions directory to the whitelist as a side effect, and that's not desirable.
Attachment #8811524 - Flags: review?(mh+mozilla)
Issues addressed.
Attachment #8811524 - Attachment is obsolete: true
Attachment #8811620 - Flags: review?(mh+mozilla)
Comment on attachment 8811620 [details] [diff] [review]
Use the correct path to the binary for comm builds in the XPCOM glue check + exceptions for Calendar and Instantbird

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

Please run though try before landing ;)

::: python/mozbuild/mozbuild/test/frontend/test_emitter.py
@@ +1178,5 @@
>          for name, path in ALLOWED_XPCOM_GLUE:
>              allowed[path].append(name)
>  
>          for path, names in allowed.iteritems():
> +            if path.starswith(('mailnews/', 'calendar/', 'extensions/purple/purplexpcom')):

heh, I typoed in my comment, that's supposed to be startswith, not starswith.
Attachment #8811620 - Flags: review?(mh+mozilla) → review+
Thanks, which try, just a build on Linux to save resources?
Flags: needinfo?(mh+mozilla)
Attachment #8811620 - Attachment is obsolete: true
Typo fixed.
Attachment #8811623 - Attachment is obsolete: true
Attachment #8811623 - Flags: review?(mh+mozilla)
Attachment #8811624 - Flags: review+
Attachment #8811620 - Attachment is obsolete: false
Attachment #8811620 - Attachment is obsolete: true
Oops. Two people working on the same thing here.
I changed:
if path.startswith(('mailnews/', 'calendar/', 'extensions/purple/purplexpcom')):

And Aleth changed(attachment 8811620 [details] [diff] [review]):
if (path.startswith('mailnews/') or
	             path.startswith('calendar/') or
                     path.startswith('extensions/purple/')):

So who's doing the try push?
Flags: needinfo?(aleth)
Oh, mine got r+ so let's go with that.

Repeating the question for Mike: which try, just a build on Linux to save resources?
(In reply to Jorg K (GMT+1) from comment #37)
> Oh, mine got r+ so let's go with that.
> 
> Repeating the question for Mike: which try, just a build on Linux to save
> resources?

Push it to m-c try.
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(aleth)
That doesn't answer the question. Which parameters? I suggested |try: -b o -p linux64 -u none -t none|.
Patch already doesn't apply any more.
Yes, that would be enough.
Green!
Keywords: checkin-needed
Attachment #8811624 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f032bd4cf861c3058f244140399f7fb2fea663
Bug 1317674 - Use the correct path to the binary for comm builds in the XPCOM glue check + exceptions for Calendar and Instantbird. r=glandium
Pushed by aleth@instantbird.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f032bd4cf8
Use the correct path to the binary for comm builds in the XPCOM glue check + exceptions for Calendar and Instantbird. r=glandium
Keywords: checkin-needed
Flags: needinfo?(philipp)
https://hg.mozilla.org/mozilla-central/rev/c5f032bd4cf8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
You need to log in before you can comment on or make changes to this bug.