1318258 - Link libical and backend with libxul after binary components have been removed

Status: RESOLVED FIXED

(Calendar :: Build Config, defect)

Description

[Jorg K (CEST = GMT+2)]

Bug 1314955 has landed, most likely we're busted big time due binary component of Calendar.

Sadly we are also currently busted by bug 1317674. I will do a special try run to see how bad the bustage is and report back here.

Comment 1

[Jorg K (CEST = GMT+2)]

Try run here. You can also see the M-C patches in the push to work around already existing bustage:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=947b30754f90994ada3ce0071e1a2f46f6001951

Comment 2

[aleth [:aleth]]

This shouldn't break builds, but it will break anything that uses libical.

Comment 3

[Jorg K (CEST = GMT+2)]

Right, the build was broken by bug 1317983. I fixed that, so here's another go:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a854a856131554c2b0aaa273d57d1769cd547b6d

Depending on how bad that looks, I might disable any libical tests for a while.

Comment 4

[Jorg K (CEST = GMT+2)]

That push looked terrible: B failed due to some build manifest out of sync (fixed by Richard), X failed due to the failing libical tests and Z failed due to two M-C bugs, one of them new in this push.

This push here temporarily disables libical tests to get a bit greener. Otherwise we risk not spotting new bustage.

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

Comment 5

[aleth [:aleth]]

(In reply to Jorg K (GMT+1) from comment #3)
> Depending on how bad that looks, I might disable any libical tests for a
> while.

Good idea.

Comment 6

[Jorg K (CEST = GMT+2)]

Pushed this again to pull in Richard's "B" fix:

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

B and X should be green after that and I'll land the "libical disable" when C-C reopens when bug 1317674 hits M-C.

/me: One day I'll need to go back to the Etherpad so record all the different bustages since the tree status is getting tight :-(

(In reply to aleth [:aleth] from comment #5)
> Good idea.
I can't believe that one of the purists agreed with a normally much-opposed temporary measure ;-)
(No offense intended).

Comment 7

[Jorg K (CEST = GMT+2)]

I understand that there are also Mozmill tests for Calendar. I didn't see them fail. Do they only run with ICalJS or did they not fail since the whole Mozmill suite crashed due to bug 1314790 and they would have come later?

Comment 8

[Jorg K (CEST = GMT+2)]

Trying a different way to disable Xpcshell tests for libical:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=87a0990d3f242fc434b0c05cdd76e6b9d0e6fe09

Comment 9

[[:MakeMyDay]]

Thanks for digging into it. xpcshell tests are run for both backends in all repos. Mozmill tests are based on the respective default backend, which is ical.js for cc and libical for aurora, beta and esr. 

So for cc, there are no changes needed, aurora and friends would need some extra care.

For diabling the xpcshell tests, it's probably better to remove/comment out the reference in calendar/test/moz.build

Comment 10

[Jorg K (CEST = GMT+2)]

Thanks. I'll see how else I can get the Xpcshell tests disabled if my current approach (see try push) doesn't work.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: Patch to *temporarily* disable Xpcshell tests using libical [landed in comment #12]

I will land this when re-opening the tree tomorrow.

Please back out this change together with the real fix.

Comment 12

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/6525bde51ec99469739c5f6592d9fd96695e8a0e
Landed temporary disabling of Xpcshell tests using libical.
(Since bug 1317674 has landed, the tree should be green again now.)

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v1

This should do it, it links the ical xpcom components into libxul. Lets hope it works:

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

Comment 14

[Jorg K (CEST = GMT+2)]

Did you consider that Mac/Win builds are currently broken?

Please check
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9ff30431d68d31281a26cc502af9456444ee8478
to see the patches you need (starting at the PK) one.

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v2

Same patch but with debug statement removed. try is broken right now due to bug 1318798 and I haven't had the patience to do a try run that applies the m-c patch.

If no one else wants to do so, we can wait until bug 1318798 is fixed and I'll do a new try run.

Comment 16

[Jorg K (CEST = GMT+2)]

Linux will work, isn't that enough? That said, here's a Win/Mac run ;-)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=74d8b017e755ab4f789ac9255ea1ee1b7d47e218

Comment 17

[Jorg K (CEST = GMT+2)]

Due to a misunderstanding, Philipp's try run got cancelled. So here is a new one on Linux only. Win and Mac are covered in the previous comment.

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

Included in the push is a M-C patch for bug 1314790 which causes Mozmill to crash.

Comment 18

[Jorg K (CEST = GMT+2)]

I cancelled all try runs since the first one failed with:
grep: /builds/slave/tb-try-c-cen-m64-0000000000000/build/objdir-tb/x86_64/dist/xpi-stage/lightning/components/libical-manifest: No such file or directory

Back to Philipp.

Comment 19

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v3

Here is a new version. The universal.mk changes are untested, so lets get a green try run before I request review. Jörg was kind enough to offer doing the try runs for me.

Comment 20

[Jorg K (CEST = GMT+2)]

Let's see how this goes. I have fixes for both current M-C bugs (bug 1318798, bug 1314790) included:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2e486a2f145163e20f3b91ed4bccf9a18fe422a0

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 8812477 [details] [diff] [review]
Fix - v3

I'm requesting review for this. Builds are successful and one X is green already indicating that libical is working an passing the tests.

Comment 22

[aleth [:aleth]]

Comment on attachment 8812477 [details] [diff] [review]
Fix - v3

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

::: calendar/base/backend/libical/calICSService.cpp
@@ +1275,5 @@
>  
>  NS_IMETHODIMP
>  calICSService::ParserWorker::Run()
>  {
> +    icalcomponent *ical = icalparser_parse_string(mString.get());

I don't understand this change.

If it is needed, isn't it also needed here: http://searchfox.org/comm-central/source/calendar/base/backend/libical/calICSService.cpp#1257

Comment 23

[Philipp Kewisch [:Fallen][📱📆]]

I can't tell you in detail why this was required, but without it I got a compiler error because of https://dxr.mozilla.org/comm-central/source/mozilla/xpcom/string/nsTPromiseFlatString.h#81.

It seems to work fine for arguments, but not for member variables. It also works fine if I #undef MOZILLA_INTERNAL_API, but then I get linker errors later on.

Comment 24

[aleth [:aleth]]

Comment on attachment 8812477 [details] [diff] [review]
Fix - v3

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

OK, let's try it.

Comment 25

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/c957f6a30b9e7e1e544194de244da1d6a1cea083
Bug 1318258 - Link libical and backend with libxul after binary components have been removed. r=aleth

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 8812477 [details] [diff] [review]
Fix - v3

Kent is in favour of uplifting this to Aurora.

Comment 27

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8812477 [details] [diff] [review]
Fix - v3

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

I can accept that. a=philipp

Comment 28

[Jorg K (CEST = GMT+2)]

Aurora, TB 52 and Calendar 5.4:
https://hg.mozilla.org/releases/comm-aurora/rev/9b480a0bede6ee27123fca79865091ff6beca007

Let's see what that does ;-)