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

RESOLVED FIXED in 5.4

Status

--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jorgk, Assigned: Fallen)

Tracking

Trunk

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years 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

2 years 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

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

Comment 3

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

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

Comment 9

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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
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

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

2 years 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

2 years 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

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

2 years 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

2 years 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

2 years 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
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

2 years 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

2 years 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

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 5.4

Updated

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

Updated

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

Comment 26

2 years 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)
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

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