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

Status: RESOLVED FIXED

(Thunderbird :: Build Config, defect)

Description

[Jorg K (CEST = GMT+2)]

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

Comment 1

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/mozilla-central/rev/621aa115c3df

Comment 2

[aleth [:aleth]]

If it's just libical, would it make sense to bite the bullet now and switch to ical.js?

Comment 3

[aleth [:aleth]]

...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.

Comment 4

[Jorg K (CEST = GMT+2)]

(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.

Comment 5

[aleth [:aleth]]

(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.

Comment 6

[aleth [:aleth]]

Attachment: Use the correct path to the binary for comm-central builds in the XPCOM glue check

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.

Comment 7

[Jorg K (CEST = GMT+2)]

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.

Comment 8

[aleth [: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).

Comment 9

[aleth [:aleth]]

Attachment: Add an xpcom glue exception for Calendar's libical

Here's a (temporary while libical is with us) patch to avoid the build bustage from comment 0.

Comment 10

[aleth [:aleth]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2c1f95b2e767e8d8b22a0ec748d8aa1d0a95effa

Comment 11

[Frank-Rainer Grahl (:frg)]

> 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?

Comment 12

[aleth [:aleth]]

(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.

Comment 13

[aleth [:aleth]]

Attachment: Add an xpcom glue exception for Instantbird 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).

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: Use the correct path to the binary for comm-central builds in the XPCOM glue check + exceptions for calendar and Instantbird

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.

Comment 15

[Jorg K (CEST = GMT+2)]

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].

Comment 16

[aleth [:aleth]]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for make check failures like https://treeherder.mozilla.org/logviewer.html#?job_id=39269184&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fa59ec4e1addd2f5e02748cc487e51c3fc41d5b

Comment 18

[Jorg K (CEST = GMT+2)]

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.

Comment 19

[Jorg K (CEST = GMT+2)]

How about:
if moz_build_app is not None and moz_build_app.startswith('../'):
?

Trying this here:
https://hg.mozilla.org/try-comm-central/rev/68daceeb88f3762ed336f0f01b1e6c748c0ac2a9#l1.44
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=955d0f2291eea74e97c90da9b13d751354c873b3

Comment 20

[aleth [:aleth]]

(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.

Comment 21

[Jorg K (CEST = GMT+2)]

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.

Comment 22

[Jorg K (CEST = GMT+2)]

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.

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: Slight change to fix "startswith" error.

Comment 24

[Jorg K (CEST = GMT+2)]

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... != []

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: Slight change to fix "startswith" error and another issue (v2).

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

Comment 26

[aleth [:aleth]]

Attachment: Use the correct path to the binary for comm builds in the XPCOM glue check + exceptions for Calendar and Instantbird

This fixes the tests on try. (Sorry jorgk for the overlap with your work, which I saw too late.)

Comment 27

[Jorg K (CEST = GMT+2)]

Comment on attachment 8811324 [details] [diff] [review]
Slight change to fix "startswith" error and another issue (v2).

Aleth'es looks better ;-)

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: Use the correct path to the binary for comm builds in the XPCOM glue check + exceptions for Calendar and Instantbird

Refreshed.

Comment 29

[Jorg K (CEST = GMT+2)]

With this patch, Thunderbird builds fine:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a6e9cea2087441d4d4e2d9357ceafeaa000a7b30

Please approve asap.

Comment 30

[Mike Hommey [:glandium]]

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.

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: Use the correct path to the binary for comm builds in the XPCOM glue check + exceptions for Calendar and Instantbird

Issues addressed.

Comment 32

[Mike Hommey [:glandium]]

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.

Comment 33

[Jorg K (CEST = GMT+2)]

Thanks, which try, just a build on Linux to save resources?

Comment 34

[aleth [:aleth]]

Attachment: Use the correct path to the binary for comm builds in the XPCOM glue check + exceptions for Calendar and Instantbird

Comment 35

[Jorg K (CEST = GMT+2)]

Attachment: Use the correct path to the binary for comm builds in the XPCOM glue check + exceptions for Calendar and Instantbird

Typo fixed.

Comment 36

[Jorg K (CEST = GMT+2)]

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?

Comment 37

[Jorg K (CEST = GMT+2)]

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?

Comment 38

[aleth [:aleth]]

(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.

Comment 39

[Jorg K (CEST = GMT+2)]

That doesn't answer the question. Which parameters? I suggested |try: -b o -p linux64 -u none -t none|.

Comment 40

[Jorg K (CEST = GMT+2)]

Patch already doesn't apply any more.

Comment 41

[Mike Hommey [:glandium]]

Yes, that would be enough.

Comment 42

[Jorg K (CEST = GMT+2)]

Attachment: Use the correct path to the binary for comm builds in the XPCOM glue check + exceptions for Calendar and Instantbird

Refreshed again.

Comment 43

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5468842a498408d3211d70ebad2fe7b1021be0ca

Comment 44

[Jorg K (CEST = GMT+2)]

Green!

Comment 45

[aleth [:aleth]]

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

Comment 46

[Pulsebot]

Pushed by [e-mail]:
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

Comment 47

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c5f032bd4cf8

Comment 48

[areya]

https://hg.mozilla.org/mozilla-central/rev/c5f032bd4cf8