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

RESOLVED FIXED in 5.4

Status

defect
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jorgk, Assigned: Fallen)

Tracking

Trunk
Dependency tree / graph

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
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
This shouldn't break builds, but it will break anything that uses libical.
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.
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
(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.
Severity: normal → critical
Component: General → Build Config
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).
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?
Flags: needinfo?(makemyday)
Trying a different way to disable Xpcshell tests for libical:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=87a0990d3f242fc434b0c05cdd76e6b9d0e6fe09
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
Flags: needinfo?(makemyday)
Thanks. I'll see how else I can get the Xpcshell tests disabled if my current approach (see try push) doesn't work.
I will land this when re-opening the tree tomorrow.

Please back out this change together with the real fix.
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.)
Keywords: leave-open
Attachment #8811841 - Attachment description: Patch to *temporarily* disable Xpcshell tests using libical → Patch to *temporarily* disable Xpcshell tests using libical [landed in comment #12]
Assignee: nobody → philipp
Depends on: 1314955
Summary: Move libical to C-C core after binary add-ons have been disabled by bug 1314955 → Link libical and backend with libxul after binary components have been removed
Posted patch Fix - v1 (obsolete) β€” β€” Splinter Review
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
Attachment #8812467 - Flags: review?(aleth)
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.
Posted patch Fix - v2 (obsolete) β€” β€” Splinter Review
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.
Attachment #8812467 - Attachment is obsolete: true
Attachment #8812467 - Flags: review?(aleth)
Attachment #8812471 - Flags: review?(aleth)
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
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.
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.
Posted patch Fix - v3 β€” β€” Splinter Review
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.
Attachment #8812471 - Attachment is obsolete: true
Attachment #8812471 - Flags: review?(aleth)
Flags: needinfo?(jorgk)
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
Flags: needinfo?(jorgk)
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.
Attachment #8812477 - Flags: review?(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
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 on attachment 8812477 [details] [diff] [review]
Fix - v3

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

OK, let's try it.
Attachment #8812477 - Flags: review?(aleth) → review+
https://hg.mozilla.org/comm-central/rev/c957f6a30b9e7e1e544194de244da1d6a1cea083
Bug 1318258 - Link libical and backend with libxul after binary components have been removed. r=aleth
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 5.4
Keywords: leave-open
Whiteboard: [update target milestone: 5.5]
Whiteboard: [update target milestone: 5.5]
Target Milestone: 5.4 → 5.5
Comment on attachment 8812477 [details] [diff] [review]
Fix - v3

Kent is in favour of uplifting this to Aurora.
Attachment #8812477 - Flags: approval-calendar-aurora?(philipp)
Comment on attachment 8812477 [details] [diff] [review]
Fix - v3

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

I can accept that. a=philipp
Attachment #8812477 - Flags: approval-calendar-aurora?(philipp) → approval-calendar-aurora+
Aurora, TB 52 and Calendar 5.4:
https://hg.mozilla.org/releases/comm-aurora/rev/9b480a0bede6ee27123fca79865091ff6beca007

Let's see what that does ;-)
Target Milestone: 5.5 → 5.4
You need to log in before you can comment on or make changes to this bug.