1466375 - Make nICEr compile as unified sources

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, enhancement, P2)

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: Bug 1466375: make nICEr and nrappkit compile as unified sources.

Review commit: https://reviewboard.mozilla.org/r/248712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248712/

Comment 2

[Eric Rescorla (:ekr)]

Comment on attachment 8982859 [details]
Bug 1466375: make nICEr and nrappkit compile as unified sources.

https://reviewboard.mozilla.org/r/248712/#review254912

Please don't just remove these. They're there for a reason. If you want to #ifdef these out in Mozilla builds, or preferentially, make them work with Git as well, that's OK.

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8982859 [details]
Bug 1466375: make nICEr and nrappkit compile as unified sources.

https://reviewboard.mozilla.org/r/248712/#review254912

These haven't been updated in a decade. I would argue that if we are going to keep them (ifdefed or not), we need to update them and keep them updated, which means making them work with _both_ git and hg. Is this really worth anyone's time in this day and age?

Comment 4

[Nils Ohlmeier [:drno]]

Attachment: Bug 1466375: avoid NRREGGET NRREGSET redefinition.

Review commit: https://reviewboard.mozilla.org/r/248930/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248930/

Comment 5

[Nils Ohlmeier [:drno]]

Comment on attachment 8983078 [details]
Bug 1466375: avoid NRREGGET NRREGSET redefinition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248930/diff/1-2/

Comment 6

[Nils Ohlmeier [:drno]]

Comment on attachment 8982859 [details]
Bug 1466375: make nICEr and nrappkit compile as unified sources.

https://reviewboard.mozilla.org/r/248712/#review254912

For future reference the idea behind these static strings is that one can easily extract these as strings from the build executable and verify which files were used in the build. But this is nowhere else done in mozilla-central. Obviously this only causes problems with unified builds like Mozilla does. But it shouldn't be too hard to re-add these if needed outside of mozilla-central.

In a conversation Ekr agreed to let go of these strings.

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 8982859 [details]
Bug 1466375: make nICEr and nrappkit compile as unified sources.

https://reviewboard.mozilla.org/r/248712/#review257922

::: media/mtransport/third_party/nICEr/src/stun/stun_build.c
(Diff revision 1)
>  */
>  
> -
> -static char *RCSSTRING __UNUSED__="$Id: stun_build.c,v 1.2 2008/04/28 18:21:30 ekr Exp $";
> -
> -#include <csi_platform.h>

Was this removal intentional?

::: media/mtransport/third_party/nICEr/src/util/ice_util.c
(Diff revision 1)
>  #include <stdarg.h>
> -
> -
> -static char *RCSSTRING __UNUSED__="$Id: ice_util.c,v 1.2 2008/04/28 17:59:05 ekr Exp $";
> -
> -#include <stdarg.h>

What about this one?

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8983078 [details]
Bug 1466375: avoid NRREGGET NRREGSET redefinition.

https://reviewboard.mozilla.org/r/248930/#review257924

::: media/mtransport/third_party/nrappkit/src/registry/registry_local.c:885
(Diff revision 2)
>      _status=0;
>    abort:
>      return(_status);
>  }
>  
> -#define NRREGGET(func, TYPE, type)                                  \
> +#define NRREGLOCGET(func, TYPE, type)                               \

I'd use the prefix "NRREGLOCAL" to match the rest of the naming, personally.

Comment 9

[Nils Ohlmeier [:drno]]

Comment on attachment 8982859 [details]
Bug 1466375: make nICEr and nrappkit compile as unified sources.

https://reviewboard.mozilla.org/r/248712/#review258094

Comment 10

[Nils Ohlmeier [:drno]]

Comment on attachment 8982859 [details]
Bug 1466375: make nICEr and nrappkit compile as unified sources.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248712/diff/1-2/

Comment 11

[Nils Ohlmeier [:drno]]

Comment on attachment 8983078 [details]
Bug 1466375: avoid NRREGGET NRREGSET redefinition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248930/diff/2-3/

Comment 12

[Nils Ohlmeier [:drno]]

Comment on attachment 8982859 [details]
Bug 1466375: make nICEr and nrappkit compile as unified sources.

https://reviewboard.mozilla.org/r/248712/#review257922

> What about this one?

Mozreview is showing this strange. There is a redundant |#include <stdarg.h>| above the RCS string, which I deleted. But this makes it look like I delete the lower one. The end result is that there is still one stdarg include.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8109f534579c
make nICEr and nrappkit compile as unified sources. r=bwc
https://hg.mozilla.org/integration/autoland/rev/56be2e8bb66a
avoid NRREGGET NRREGSET redefinition. r=bwc

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8109f534579c
https://hg.mozilla.org/mozilla-central/rev/56be2e8bb66a