935857 - Replace MOZ_RTSP with proper NECKO_PROTOCOL handling

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

Bug 831645 added a new protocol to necko, but instead of using the existing infrastructure (NETWORK_PROTOCOLS), it added its own variable.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Replace MOZ_RTSP with proper NECKO_PROTOCOL handling

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 828528 [details] [diff] [review]
Replace MOZ_RTSP with proper NECKO_PROTOCOL handling

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

::: b2g/installer/Makefile.in
@@ +49,5 @@
>  ifneq (,$(filter WINNT Darwin Android,$(OS_TARGET)))
>  DEFINES += -DMOZ_SHARED_MOZGLUE=1
>  endif
>  
> +ifneq (,$(filter rtsp,$(NECKO_PROTOCOLS)))

Can you do this in moz.build?

::: netwerk/protocol/moz.build
@@ +3,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +PARALLEL_DIRS += sorted(CONFIG['NECKO_PROTOCOLS'].split())

Is sorted() exposed?

Comment 3

[Mike Hommey [:glandium]]

(In reply to :Ms2ger from comment #2)
> > +PARALLEL_DIRS += sorted(CONFIG['NECKO_PROTOCOLS'].split())
> 
> Is sorted() exposed?

yes.

Comment 4

[Mike Hommey [:glandium]]

(In reply to :Ms2ger from comment #2)
> > +ifneq (,$(filter rtsp,$(NECKO_PROTOCOLS)))
> 
> Can you do this in moz.build?

Actually, no, because b2g/installer is not a moz.build directory, although there is a moz.build, but it's not recursed into.
https://mxr.mozilla.org/mozilla-central/source/b2g/moz.build#7

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 828528 [details] [diff] [review]
Replace MOZ_RTSP with proper NECKO_PROTOCOL handling

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

::: netwerk/base/src/Makefile.in
@@ +10,3 @@
>  LOCAL_INCLUDES  += -I$(topsrcdir)/netwerk/protocol/rtsp/controller
>  LOCAL_INCLUDES  += -I$(topsrcdir)/netwerk/protocol/rtsp/rtsp
>  endif

While you're here, can you move these LOCAL_INCLUDES to moz.build?

::: netwerk/streamconv/converters/moz.build
@@ +16,5 @@
>      'nsTXTToHTMLConv.cpp',
>      'nsUnknownDecoder.cpp',
>  ]
>  
> +if 'ftp' in CONFIG['NECKO_PROTOCOLS'].split():

I wonder how much value exposing certain config variables as sets to Python would be. AC_SUBST_SET - Value would be string to make like AC_SUBST, but set to Python.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Add AC_SUBST_SET to create a set() of strings in config.status and use it for NECKO_PROTOCOLS

I think this is a good idea and has value. Let's try that.

Comment 7

[Mike Hommey [:glandium]]

Attachment: Replace MOZ_RTSP with proper NECKO_PROTOCOL handling

Updated with gps' comments and rebased against the other patch ; only interested in review of the .h/.cpp changes.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 829009 [details] [diff] [review]
Add AC_SUBST_SET to create a set() of strings in config.status and use it for NECKO_PROTOCOLS

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

::: build/autoconf/config.status.m4
@@ +116,5 @@
>  
>  cat >> $CONFIG_STATUS <<\EOF
>  ] ]
>  
> +substs = [(name[1:-1], value[1:-1] if isinstance(value, types.StringTypes) else value) for name, value in [

All the literals should be str. Why bring in types.StringTypes?

::: python/mozbuild/mozbuild/backend/configenvironment.py
@@ +156,5 @@
> +            if not isinstance(v, StringTypes):
> +                if isinstance(v, Iterable):
> +                    type(v)(decode(i) for i in v)
> +            elif not isinstance(v, text_type):
> +                v = decode(v)

This feels like we're missing some asserts to ensure unexpected conditions are never hit.

Comment 9

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #8)
> All the literals should be str. Why bring in types.StringTypes?

Future-proofing, since we want to use unicode literals, in the end.

Comment 10

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 829011 [details] [diff] [review]
Replace MOZ_RTSP with proper NECKO_PROTOCOL handling

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

Thanks for you help to clean this up. It's really cool.

Comment 11

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/30a667407079
https://hg.mozilla.org/integration/mozilla-inbound/rev/260fbed9b3ef

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/30a667407079
https://hg.mozilla.org/mozilla-central/rev/260fbed9b3ef