Give libevent its own moz.build file

RESOLVED FIXED in Firefox 44

Status

()

Core
Build Config
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla44
Points:
---

Firefox Tracking Flags

(firefox43 affected, firefox44 fixed)

Details

Attachments

(6 attachments)

(Assignee)

Description

2 years ago
Most of the warnings in ipc/chromium/ come from libevent/. And libevent/ is true third-party code that we sometimes update from upstream and so shouldn't modify, unlike the rest of ipc/chromium/ which we forked. Therefore we should allow warnings for libevent/ but disallow them elsewhere in ipc/chromium/, which requires giving libevent/ its own moz.build file.
(Assignee)

Updated

2 years ago
Blocks: 1181026
(Assignee)

Comment 1

2 years ago
Created attachment 8663551 [details] [diff] [review]
(part 1) - Combine handling of BSDs in ipc/chromium/moz.build
Attachment #8663551 - Flags: review?(mshal)
(Assignee)

Comment 2

2 years ago
Created attachment 8663552 [details] [diff] [review]
(part 2) - Move Linux-specific code ipc/chromium/moz.build

Just so the OS ordering at the top matches the OS ordering at the bottom.
Attachment #8663552 - Flags: review?(mshal)
(Assignee)

Comment 3

2 years ago
Created attachment 8663553 [details] [diff] [review]
(part 3) - Move Android-specific code ipc/chromium/moz.build
Attachment #8663553 - Flags: review?(mshal)
(Assignee)

Comment 4

2 years ago
Created attachment 8663554 [details] [diff] [review]
(part 4) - Factor out include handling in ipc/chromium/moz.build
Attachment #8663554 - Flags: review?(mshal)
(Assignee)

Comment 5

2 years ago
Created attachment 8663555 [details] [diff] [review]
(part 5) - Give libevent its own moz.build file

I originally tried putting it in ipc/chromium/src/third_party/libevent/, but
that directory already has a Makefile.in file, which caused problems, so I
moved it down one directory.
Attachment #8663555 - Flags: review?(mshal)
Comment on attachment 8663551 [details] [diff] [review]
(part 1) - Combine handling of BSDs in ipc/chromium/moz.build

>+        # of NoOp. 

nit: trailing whitespace
Attachment #8663551 - Flags: review?(mshal) → review+

Updated

2 years ago
Attachment #8663552 - Flags: review?(mshal) → review+

Updated

2 years ago
Attachment #8663553 - Flags: review?(mshal) → review+
Comment on attachment 8663554 [details] [diff] [review]
(part 4) - Factor out include handling in ipc/chromium/moz.build

>+    LOCAL_INCLUDES += sorted([
>         'src/third_party/libevent',
>         'src/third_party/libevent/include',
>-    ]
>+        'src/third_party/libevent/' + libevent_include_suffix,
>+    ])

I'm a little wary of using sorted() here, since effectively that means the list of explicit LOCAL_INCLUDES can be unsorted, unlike elsewhere in moz.build. I realize you used it because of the variable libevent_include_suffix, but you could also work around that by using a separate 'LOCAL_INCLUDES += ' statement. That may look messier for such a short list like this though, so I'll leave it up to you :).
Attachment #8663554 - Flags: review?(mshal) → review+
Comment on attachment 8663555 [details] [diff] [review]
(part 5) - Give libevent its own moz.build file

>diff --git a/ipc/chromium/src/third_party/moz.build b/ipc/chromium/src/third_party/moz.build
>+os_win = 0
>+os_posix = 0
>+os_macosx = 0
>+os_bsd = 0
>+os_linux = 0
>+
>+if CONFIG['OS_ARCH'] == 'Darwin':
>+    os_macosx = 1
>+    libevent_include_suffix = 'mac'
>+elif CONFIG['OS_ARCH'] in ['DragonFly', 'FreeBSD', 'GNU_kFreeBSD',
>+                           'NetBSD', 'OpenBSD']:
>+    os_bsd = 1
>+    libevent_include_suffix = 'bsd'
>+else:
>+    os_linux = 1
>+    if CONFIG['OS_TARGET'] == 'Android':
>+        libevent_include_suffix = 'android'
>+    else:
>+        libevent_include_suffix = 'linux'

Is it possible to share this code in a ipc/chromium/arches.mozbuild or some such file? I don't want to block you on this, but I can see these easily getting out of sync.

>+LOCAL_INCLUDES += sorted([
>+    'libevent',
>+    'libevent/include',
>+    'libevent/' + libevent_include_suffix,
>+])

This can maybe go in the shared file as well? I forget if that's easy to do now or if it's still a pain to do relative LOCAL_INCLUDES from an included mozbuild file. If it's ugly, don't worry about it.

Overall it looks good - thanks for taking this on!
Attachment #8663555 - Flags: review?(mshal) → review+
(Assignee)

Comment 10

2 years ago
Created attachment 8663940 [details] [diff] [review]
(part 6) - Factor out common libevent moz.build stuff

A little clunky, but it avoids the duplication.
Attachment #8663940 - Flags: review?(mshal)
(Assignee)

Comment 11

2 years ago
> I'm a little wary of using sorted() here, since effectively that means the
> list of explicit LOCAL_INCLUDES can be unsorted, unlike elsewhere in
> moz.build.

I tried changing it to the way you suggested, but I started writing a comment to explain why it was necessary and I figured that would end up being more complicated. Especially given that it's a list with only three elements.

I'll factor out the common stuff like you suggested. I'll do it in an additional patch.
(Assignee)

Updated

2 years ago
No longer blocks: 1181026
Comment on attachment 8663940 [details] [diff] [review]
(part 6) - Factor out common libevent moz.build stuff

Looks good! Too bad the libevent_path_prefix isn't some automatic variable set by mozbuild when including helper files like this.
Attachment #8663940 - Flags: review?(mshal) → review+
You need to log in before you can comment on or make changes to this bug.