Closed Bug 803480 Opened 7 years ago Closed 6 years ago

revise supported platforms for necko wifi

Categories

(Firefox Build System :: General, defect)

x86_64
FreeBSD
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla25

People

(Reporter: jbeich, Assigned: jbeich)

References

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
This also fails early on Linux with --disable-dbus but no --disable-necko-wifi in .mozconfig.
Attachment #673150 - Flags: review?(doug.turner)
- restore accidentally removed `if test $NECKO_WIFI; then'
- update comment
Attachment #673150 - Attachment is obsolete: true
Attachment #673150 - Flags: review?(doug.turner)
Attachment #673513 - Flags: review?(doug.turner)
Attachment #673513 - Flags: review?(doug.turner) → review?(ted)
Comment on attachment 673513 [details] [diff] [review]
fail with --disable-dbus on any Unix, v2

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

Sorry for the review delay.

::: configure.in
@@ +8236,5 @@
> +    Darwin)
> +      ;;
> +    SunOS)
> +      ;;
> +    WINNT)

You can combine these all into one case line like:
Android|Darwin|SunOS|WINNT)
  ;;

@@ +8244,5 @@
> +      NECKO_WIFI=
> +      ;;
> +    *)
> +      if test -z "$MOZ_ENABLE_DBUS"; then
> +        AC_MSG_ERROR([Necko WiFi scanning needs DBus on your platform, remove --disable-dbus or use --disable-necko-wifi])

This doesn't seem really accurate to me. If someone adds support for a new platform and doesn't add it to the list above, they'll hit this error message. Can we not enumerate the platforms where we require DBus support instead?
Attachment #673513 - Flags: review?(ted) → review-
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> You can combine these all into one case line like:
> Android|Darwin|SunOS|WINNT)

Even if they don't share implementation? Linux, BSDs and other Unices (except previously mentioned) fall back to query NetworkManager via DBus. My patch simply adjusts the requirement as specified in netwerk/wifi/Makefile.in.
You have a bunch of no-op cases in the case statement. You can simply fold them together. The main reason for the r- is that we've lumped all other OSes into the "requires DBus" bin, and that doesn't seem like the right way to address this.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> The main reason for the r- is that we've lumped all other OSes into
> the "requires DBus" bin, and that doesn't seem like the right way to
> address this.

What else is there besides "enumerating", "lumping" and automatically disabling necko-wifi without dbus ? Are you against the "lumping" in netwerk/wifi/Makefile.in introduced by bug 799391, too?
That Makefile reads differently. It says "If this platform has DBus enabled, use the DBus backend", which makes sense. Your configure changes say "if this platform is not one of these other platforms I've listed, and you don't have DBus, error". I would prefer that we explicitly list platforms that we know support DBus, whether that's by putting them all in this case statement, or by adding some intermediate variable (MOZ_WIFI_DBUS?) that we can set in the target platform checks earlier in configure.
Attached patch sane defaults (obsolete) — Splinter Review
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> I would prefer that we explicitly list platforms that we
> know support DBus

NetworkManager (unlike DBus) doesn't exist outside of Linux ecosystem atm.
Attachment #673513 - Attachment is obsolete: true
Attachment #775166 - Flags: review?(ted)
Blocks: 893397
Attached patch sane defaults (obsolete) — Splinter Review
try again with more diff context
Attachment #775166 - Attachment is obsolete: true
Attachment #775166 - Flags: review?(ted)
Attachment #775179 - Flags: review?(ted)
Comment on attachment 775179 [details] [diff] [review]
sane defaults

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

::: configure.in
@@ +8481,5 @@
>  dnl
> +
> +case "$OS_TARGET" in
> +  OS2)
> +    dnl OS/2 implementation of Necko-WiFi support will be added in bug 506566

I would just drop this block. I don't think anyone is actively working on the OS/2 port anyway, and having a comment about something that might be implemented is silly.
Attachment #775179 - Flags: review?(ted) → review+
Assignee: nobody → jbeich
Attached patch sane defaultsSplinter Review
dropped OS/2 block, keeping r+
Attachment #775179 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b794c01d9e70
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Funnily, i tried to do somewhat the same in bug #653902, and at that time it got WONTFIXed.. guess things change.
Blocks: 901251
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.