803480 - revise supported platforms for necko wifi

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Jan Beich]

Attachment: don't fail with dbus on non-Linux

This also fails early on Linux with --disable-dbus but no --disable-necko-wifi in .mozconfig.

Comment 2

[Jan Beich]

Attachment: fail with --disable-dbus on any Unix, v2

- restore accidentally removed `if test $NECKO_WIFI; then'
- update comment

Comment 3

[(not currently active) Ted Mielczarek]

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?

Comment 4

[Jan Beich]

(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.

Comment 5

[(not currently active) Ted Mielczarek]

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.

Comment 6

[Jan Beich]

(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?

Comment 7

[(not currently active) Ted Mielczarek]

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.

Comment 8

[Jan Beich]

Attachment: sane defaults

(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.

Comment 9

[Jan Beich]

Attachment: sane defaults

try again with more diff context

Comment 10

[(not currently active) Ted Mielczarek]

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.

Comment 11

[Jan Beich]

Attachment: sane defaults

dropped OS/2 block, keeping r+

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b794c01d9e70

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b794c01d9e70

Comment 14

[Landry Breuil (:gaston)]

Funnily, i tried to do somewhat the same in bug #653902, and at that time it got WONTFIXed.. guess things change.