After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 774641 - gst-editing-services: Enable building with MSVC
gst-editing-services: Enable building with MSVC
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-editing-services
git master
Other Windows
: Normal enhancement
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-11-17 19:13 UTC by Scott D Phillips
Modified: 2016-11-18 19:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH gst-editing-services 1/4] parse: Don't #include <unistd.h> (1.19 KB, patch)
2016-11-17 19:14 UTC, Scott D Phillips
committed Details | Review
[PATCH gst-editing-services 2/4] Pass gint/guint pointers instead of enum pointers (1.88 KB, patch)
2016-11-17 19:15 UTC, Scott D Phillips
committed Details | Review
[PATCH gst-editing-services 3/4] Cast away const from GstMetaInfo in *_get_meta_info() functions (1.33 KB, patch)
2016-11-17 19:15 UTC, Scott D Phillips
committed Details | Review
[PATCH gst-editing-services 4/4] Enable building with MSVC (14.47 KB, patch)
2016-11-17 19:15 UTC, Scott D Phillips
none Details | Review
[PATCH gst-editing-services v2 4/4] Enable building with MSVC (14.98 KB, patch)
2016-11-18 01:48 UTC, Scott D Phillips
committed Details | Review
[PATCH gst-editing-services 5/5] make: include common/win32.mak (1.77 KB, patch)
2016-11-18 18:36 UTC, Scott D Phillips
committed Details | Review
[PATCH gst-common] check-exports: Change symbol regex to work for gst-editing-services (1.23 KB, patch)
2016-11-18 18:38 UTC, Scott D Phillips
committed Details | Review

Description Scott D Phillips 2016-11-17 19:13:45 UTC
I don't promise everything is functioning properly with these patches, but at least it builds.
Comment 1 Scott D Phillips 2016-11-17 19:14:35 UTC
Created attachment 340178 [details] [review]
[PATCH gst-editing-services 1/4] parse: Don't #include <unistd.h>
Comment 2 Scott D Phillips 2016-11-17 19:15:01 UTC
Created attachment 340179 [details] [review]
[PATCH gst-editing-services 2/4] Pass gint/guint pointers instead of enum pointers
Comment 3 Scott D Phillips 2016-11-17 19:15:16 UTC
Created attachment 340180 [details] [review]
[PATCH gst-editing-services 3/4] Cast away const from GstMetaInfo in *_get_meta_info() functions
Comment 4 Scott D Phillips 2016-11-17 19:15:34 UTC
Created attachment 340181 [details] [review]
[PATCH gst-editing-services 4/4] Enable building with MSVC
Comment 5 Thibault Saunier 2016-11-17 19:28:53 UTC
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?
Comment 6 Thibault Saunier 2016-11-17 19:29:54 UTC
Review of attachment 340179 [details] [review]:

hm, OK :)
Comment 7 Thibault Saunier 2016-11-17 19:31:56 UTC
Review of attachment 340180 [details] [review]:

OK, same as everywhere else :)
Comment 8 Thibault Saunier 2016-11-17 19:35:18 UTC
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 :)
Comment 9 Scott D Phillips 2016-11-17 19:36:32 UTC
(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.
Comment 10 Thibault Saunier 2016-11-17 19:38:49 UTC
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 :)
Comment 11 Scott D Phillips 2016-11-17 20:44:51 UTC
(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.
Comment 12 Tim-Philipp Müller 2016-11-17 23:21:14 UTC
> > 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.
Comment 13 Scott D Phillips 2016-11-18 00:12:36 UTC
(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.
Comment 14 Scott D Phillips 2016-11-18 01:48:14 UTC
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
Comment 15 Thibault Saunier 2016-11-18 12:13:37 UTC
Review of attachment 340201 [details] [review]:

OK
Comment 16 Thibault Saunier 2016-11-18 12:20:33 UTC
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
Comment 17 Sebastian Dröge (slomo) 2016-11-18 15:55:28 UTC
The .def file should get the "update-exports" / "check-exports" magic we have in core/base, to prevent forgetting to update them.
Comment 18 Scott D Phillips 2016-11-18 18:36:58 UTC
Created attachment 340261 [details] [review]
[PATCH gst-editing-services 5/5] make: include common/win32.mak
Comment 19 Scott D Phillips 2016-11-18 18:38:32 UTC
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?