Closed Bug 1466375 Opened 2 years ago Closed 2 years ago

Make nICEr compile as unified sources

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: drno, Assigned: drno)

Details

Attachments

(2 files)

No description provided.
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 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 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.
Attachment #8983078 - Flags: review?(docfaraday)
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?
Attachment #8982859 - Flags: review?(docfaraday) → review+
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.
Attachment #8983078 - Flags: review?(docfaraday) → review+
Comment on attachment 8982859 [details]
Bug 1466375: make nICEr and nrappkit compile as unified sources.

https://reviewboard.mozilla.org/r/248712/#review258094
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.
Pushed by drno@ohlmeier.org:
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
https://hg.mozilla.org/mozilla-central/rev/8109f534579c
https://hg.mozilla.org/mozilla-central/rev/56be2e8bb66a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.