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

RESOLVED FIXED in 5.4

Status

Calendar
Build Config
--
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Jorg K (GMT+1), Assigned: Fallen)

Tracking

Trunk

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
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.
(Reporter)

Comment 1

a year ago
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

a year ago
This shouldn't break builds, but it will break anything that uses libical.
(Reporter)

Comment 3

a year ago
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.
(Reporter)

Comment 4

a year ago
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

a year ago
(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
(Reporter)

Comment 6

a year ago
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).
(Reporter)

Comment 7

a year ago
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)
(Reporter)

Comment 8

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

Comment 9

a year ago
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)
(Reporter)

Comment 10

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

Comment 11

a year ago
Created attachment 8811841 [details] [diff] [review]
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.
(Reporter)

Comment 12

a year ago
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
(Reporter)

Updated

a year ago
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)

Updated

a year ago
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
(Assignee)

Comment 13

a year ago
Created attachment 8812467 [details] [diff] [review]
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
Attachment #8812467 - Flags: review?(aleth)
(Reporter)

Comment 14

a year ago
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.
(Assignee)

Comment 15

a year ago
Created attachment 8812471 [details] [diff] [review]
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.
Attachment #8812467 - Attachment is obsolete: true
Attachment #8812467 - Flags: review?(aleth)
Attachment #8812471 - Flags: review?(aleth)
(Reporter)

Comment 16

a year ago
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
(Reporter)

Comment 17

a year ago
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.
(Reporter)

Comment 18

a year ago
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.
(Assignee)

Comment 19

a year ago
Created attachment 8812477 [details] [diff] [review]
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.
Attachment #8812471 - Attachment is obsolete: true
Attachment #8812471 - Flags: review?(aleth)
Flags: needinfo?(jorgk)
(Reporter)

Comment 20

a year ago
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)
(Reporter)

Comment 21

a year ago
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 22

a year ago
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
(Assignee)

Comment 23

a year ago
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

a year ago
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+

Comment 25

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

Updated

a year ago
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → 5.4

Updated

a year ago
Keywords: leave-open
Whiteboard: [update target milestone: 5.5]
(Assignee)

Updated

a year ago
Whiteboard: [update target milestone: 5.5]
Target Milestone: 5.4 → 5.5
(Reporter)

Comment 26

a year ago
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)
(Assignee)

Comment 27

a year ago
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+
(Reporter)

Comment 28

a year ago
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.