1486043 - libprio does not compile on MSVC

Status: RESOLVED WONTFIX

(Firefox :: General, defect)

Description

[Robert Helmer [:rhelmer]]

So there's this tier-2 MSVC-only builder, and MSVC only supports C90 unlike our other compilers so we're hitting a missing feature (variable length arrays)

We're in the process of switching to Clang on Windows but are keeping this running in case we need to back out of that decision at the last minute):

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=195697225

Per https://developer.mozilla.org/en-US/docs/Mozilla/Supported_build_configurations - "Tier-2 platforms are those platforms that the Mozilla community actively maintains. Breakage or regressions in these platforms does not immediately close the tree; developers who break these platforms are expected to work with platform maintainers to fix problems, and may be required to back out their changes if a fix cannot be found"

I've filed https://github.com/mozilla/libprio/issues/17 to track this, the current bug will be used to update the vendored libprio in Firefox.

Comment 1

[Robert Helmer [:rhelmer]]

After chatting with Ted, it sounds like confidence is high we will ship Windows with Clang and not go back to MSVC, so probably a better tack would be to introduce a MOZ_LIBPRIO config option and explicitly not build the PrioEncoder DOM stuff if this is not set.

It's likely that Tier-3 platforms (Linux on weird arches, BSD, whatever Solaris has morphed into now, etc) will need to disable this and we don't want to spend any time on that (the maintainers could just use the flag)

(In reply to Robert Helmer [:rhelmer] from comment #0)
> I've filed https://github.com/mozilla/libprio/issues/17 to track this, the
> current bug will be used to update the vendored libprio in Firefox.

I *do* think that MSVC support is important for the upstream libprio, just it's not a priority for Firefox, so I'll leave the PR open (I started a patch but it's a bunch of silly work I'd rather not do right now)

Comment 2

[(not currently active) Ted Mielczarek]

To clarify: I suggested disabling this feature in moz.configure when compiling with MSVC, but not providing an actual configure option for it. Other platforms that can't build the code can just disable it using the same mechanism.

Comment 3

[Robert Helmer [:rhelmer]]

(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #2)
> To clarify: I suggested disabling this feature in moz.configure when
> compiling with MSVC, but not providing an actual configure option for it.
> Other platforms that can't build the code can just disable it using the same
> mechanism.

Thanks for the clarification! I've made a patch to that effect in bug 1485946, let's leave the current bug open for when libprio gets MSVC support (which might happen soon per the upstream issue).

Comment 4

[Robert Helmer [:rhelmer]]

I think we actually don't care about fixing this for Firefox.

It might be helpful for upstream libprio but even there it seems like enough of a pain that we might want to say clang+Windows is the supported compiler, and if someone actually needs MSVC support down the line they could contribute etc.