GNOME Bugzilla – Bug 774641
gst-editing-services: Enable building with MSVC
Last modified: 2016-11-18 19:56:24 UTC
I don't promise everything is functioning properly with these patches, but at least it builds.
Created attachment 340178 [details] [review] [PATCH gst-editing-services 1/4] parse: Don't #include <unistd.h>
Created attachment 340179 [details] [review] [PATCH gst-editing-services 2/4] Pass gint/guint pointers instead of enum pointers
Created attachment 340180 [details] [review] [PATCH gst-editing-services 3/4] Cast away const from GstMetaInfo in *_get_meta_info() functions
Created attachment 340181 [details] [review] [PATCH gst-editing-services 4/4] Enable building with MSVC
Review of attachment 340178 [details] [review]: Looks like they came up with a more complex solution in GStreamer core: https://phabricator.freedesktop.org/diffusion/GST/browse/master/gst/parse/meson.build;fa3eac2d28e0006f7bf555c866b1daefd15c278b$22 any downside of disabling unistd on unix systems?
Review of attachment 340179 [details] [review]: hm, OK :)
Review of attachment 340180 [details] [review]: OK, same as everywhere else :)
Review of attachment 340181 [details] [review]: Why didn't we need the .def file before? GES is known to work on windows and I know various people using it VS (anyway afaiu we want it indeed). ::: meson.build @@ -30,3 @@ # If a warning is harmless but hard to fix, use '/woXXXX' so it's shown once # NOTE: Only add warnings here if you are sure they're spurious -if cc.get_id() == 'msvc' Please use the new add_project_arguments() now and bum meson dependency to 0.36.0 :)
(In reply to Thibault Saunier from comment #5) > Review of attachment 340178 [details] [review] [review]: > > Looks like they came up with a more complex solution in GStreamer core: > https://phabricator.freedesktop.org/diffusion/GST/browse/master/gst/parse/ > meson.build;fa3eac2d28e0006f7bf555c866b1daefd15c278b$22 any downside of > disabling unistd on unix systems? No, your lexer code either uses posix stuff from unistd.h or it does not, so we'll know if this is good or not at build time. flex always includes unistd.h for your convenience AFAICT. Disabling including it just with msvc is probably asking for trouble because at some point you might start using some unistd stuff. You'll see everything compile and work on linux but then the build breaks on windows. If we just never include unistd then won't get that situation.
Review of attachment 340178 [details] [review]: > No, your lexer code either uses posix stuff from unistd.h or it does not, so we'll know if this is good or not at build time. flex always includes unistd.h for your convenience AFAICT. Disabling including it just with msvc is probably asking for trouble because at some point you might start using some unistd stuff. You'll see everything compile and work on linux but then the build breaks on windows. If we just never include unistd then won't get that situation. OK then :)
(In reply to Thibault Saunier from comment #8) > Review of attachment 340181 [details] [review] [review]: > > Why didn't we need the .def file before? GES is known to work on windows and > I know various people using it VS (anyway afaiu we want it indeed). That's a good question, I guess we would have to take a look at how it is being built to see for sure. The def file stuff (or else peppering __declspec(dllexport) all over) is only needed when you make a .dll, so maybe they're statically linking? > Please use the new add_project_arguments() now and bum meson dependency to > 0.36.0 :) Will do.
> > Why didn't we need the .def file before? GES is known to work on windows and > > I know various people using it VS (anyway afaiu we want it indeed). > > That's a good question, I guess we would have to take a look at how it is > being built to see for sure. The def file stuff (or else peppering > __declspec(dllexport) all over) is only needed when you make a .dll, so > maybe they're statically linking? I suspect mingw like gcc exports symbols by default, while MSVC does not.
(In reply to Tim-Philipp Müller from comment #12) > I suspect mingw like gcc exports symbols by default, while MSVC does not. Right, all this .def/dllexport stuff is only for MSVC.
Created attachment 340201 [details] [review] [PATCH gst-editing-services v2 4/4] Enable building with MSVC Changes since v1: - use add_project_arguments instead of add_global_arguments
Review of attachment 340201 [details] [review]: OK
commit 9feee792c50f2cf0f65b239557a948a8ac8eef59 Author: Scott D Phillips <scott.d.phillips@intel.com> Date: Thu Nov 17 10:31:50 2016 -0800 Enable building with MSVC https://bugzilla.gnome.org/show_bug.cgi?id=774641 commit 3319ed17d8be34281f95993263e7186a8c444654 Author: Scott D Phillips <scott.d.phillips@intel.com> Date: Thu Nov 17 10:40:05 2016 -0800 Cast away const from GstMetaInfo in *_get_meta_info() functions MSVC warns about the const in the implicit argument conversion in the calls to g_once_init_{enter,leave}. It's OK so explicitly cast it. https://bugzilla.gnome.org/show_bug.cgi?id=774641 commit f203c01af2f441ebbbb91b4d8797e39dda42c371 Author: Scott D Phillips <scott.d.phillips@intel.com> Date: Thu Nov 17 10:39:01 2016 -0800 Pass gint/guint pointers instead of enum pointers The underlying integer type for enums are implementation defined and may not be the same size as gint/guint. So implicitly casting from pointers- to-enum-types to pointers-to-int-types is unsafe. MSVC warns on these. https://bugzilla.gnome.org/show_bug.cgi?id=774641 commit f1dd6f4226f72e7a1e12931ac03fc683e9a0b67d Author: Scott D Phillips <scott.d.phillips@intel.com> Date: Thu Nov 17 10:35:50 2016 -0800 parse: Don't #include <unistd.h> It isn't needed and isn't present in non-posix environments like windows with MSVC or mingw. https://bugzilla.gnome.org/show_bug.cgi?id=774641
The .def file should get the "update-exports" / "check-exports" magic we have in core/base, to prevent forgetting to update them.
Created attachment 340261 [details] [review] [PATCH gst-editing-services 5/5] make: include common/win32.mak
Created attachment 340262 [details] [review] [PATCH gst-common] check-exports: Change symbol regex to work for gst-editing-services @slomo, this is the easiest way to make this work but it's a bit inelegant. Think it's worth reworking the check-exports script to have a configurable prefix or something?