Menu

#313 Removal of #ifndef MPG123_NO_CONFIGURE breaks build

1.28.x
closed-fixed
nobody
None
5
2021-07-12
2021-06-05
manx
No

Removal #ifndef MPG123_NO_CONFIGURE in mpg123.h.in breaks the possibility to build libmpg123 with native MSVC. It now requires cmake or autotools to even be usable. Forcing cmake is not a viable solution to support Visual Studio, IMHO. Cmake solves precisely none of our requirements on a build system in OpenMPT and libopenmpt.
I do not desire Visual Studio project files in mpg123. We will roll our own anyways. What should be possible though is building libmpg123 without any weird requirements on the build system (like modifying an autoconf template file).

Discussion

  • Thomas Orgis

    Thomas Orgis - 2021-06-07

    (The notification mail from sf got stuck someplace and I just got it this morning.)

    I was not aware of externally curated projects that rely on our hacks for that. I might have forgotten.

    Funny enough, the recent business in the CMake port was about fixing up libopenmpt build in vcpkg. Now I got the original here!

    Anyhow, care to check current svn trunk? I had traces of MPG123_NO_CONFIGURE around in the other libs, anyway. So I completed that.

    You need to define MPG123_API_VERSION for a proper build. Otherwise, you might see issues with @MPG123_API_VERSION@ in the effective header. Ugly.

     
    • manx

      manx - 2021-06-07

      Well no, svn trunk does not work as intended because now, #if (!defined MPG123_NO_LARGENAME) && ((defined _FILE_OFFSET_BITS) || (defined MPG123_LARGESUFFIX)) is evaluated even on platforms like MSVC where _FILE_OFFSET_BITS has no relevance. If this is defined by accident, we now get "64"-suffixed names. I would prefer to keep MPG123_NO_CONFIGURE around the whole largefile ifdef-ery, like it was in 1.27. Keeping it exactly as it was would also probably reduce problems for other users that we do not know about.

      We do not rely MPG123_API_VERSION because we know the mpg123 version we build against (for MSVC builds at least). Thus, we do not really care if it's a bogus string. Given that we now could define it I'll probably do that though ;-).

      Frankly, I'd rather prefer to see a new, clean interface header for libmpg123 that does not rely on autoconf, off_t, ssize_t, or the encoding of pathnames at all.
      Your commit message in r4920 seems to indicate that you consider to got somewhat in the direction anyway, right?
      I would go some steps further though:
      * Depending on system-specific and build-dependent types in the API and ABI is problematic in general (which is why the whole libmpg123 largefile macros exist at all). In libopenmpt, we handle that by defining our own simple FILE*-like callback API and only ever use that (somewhat akin to what libmpg123 supports with mpg123_replace_reader_handle). The interface just unconditionally exposes a 64bit API, avoiding all possible conflicts in that area. We also provide simple helper functions that allow implementing our callbacks in client code for common file API. See https://github.com/OpenMPT/openmpt/blob/master/libopenmpt/libopenmpt.h#L293, https://github.com/OpenMPT/openmpt/blob/master/libopenmpt/libopenmpt_stream_callbacks_file.h, and https://github.com/OpenMPT/openmpt/blob/master/libopenmpt/libopenmpt_stream_callbacks_fd.h. This also shifts the problem of pathname encoding out-of-scope for the library and into the responsibility of client code (which simplifies the library in that regard).
      * I am not sure which platforms libmpg123 supports that still do not support C99. And even if they do not, I guess all platforms should at least have a rudimentary stdint.h even when not fully supporting C99.
      * Defining ssize_t (as done for MSVC) is problematic if another library decides to do the same thing. I would rather use always ptrdiff_t or just int64_t.

      If desired, I could help working on that. We maybe should also move this discussion to a separate issue.

       
  • Thomas Orgis

    Thomas Orgis - 2021-06-07

    Oh, and you sure you got the largefile stuff handled correctly? In MSVC, off_t is always 32 bit and you should define MPG23_NO_LARGENAME.

     
  • Thomas Orgis

    Thomas Orgis - 2021-06-07

    Yes, having _FILE_OFFSET_BITS set in an MSVC environment is not a good idea. That is why I wrote things so that you can do

    #define MPG123_API_VERSION 46
    #define MPG123_NO_CONFIGURE
    #define MPG123_NO_LARGENAME
    #include "mpg123.h.in"
    

    now. Isn't that OK? It's more flexible, you can opt into the _64 names without configure now.

    I remember some compiler error for the vcpkg folks because of the @MPG123_API_VERSION@ being present literally, hence it can be defined, too. Basically, I am fine with offering #define alternatives for all the @REPLACEMENT@ stuff so that you can settle things in the preprocessor.

    About the ssize_t thing. Would a #ifndef ssize_t #define ssize_t thing be more robust? I have no inclination to suffer too much pain because Microsoft decided that a signed size-like type does not make sense.

    So far, I prefer to have the header not require anything besides basic C90 … and well, ssize_t and off_t according to POSIX, if it is not necessary. Pleasing a platform that misunderstood off_t even more than Linux and that I also don't test myself, does not feel that necessary. If I'd write something new altogether, I sure would raise the bar to stdint at least. I am not ruling it out for mpg123, which has been around since 1995 or so, but so far it is unnecessary for the POSIX platforms (including Windows with the MinGW environment, which does handle off_t).

    About how to define APIs without relying on these problematic types: That ship has sailed. I won't change the API as I am serious about keeping compatibility. You do see that I try to patch things up, with adding functions with some fixed semantics. But they have differing names that might be glossed over with defines (see MPG123_ENUM_API, also as extreme example of paranoia).

    Beginning freshly, I might just cave in and use int64_t instead of off_t, but I still think that sucks as the library API should slot in nicely into what the OS API for files is. Now Windows is a very specific OS with no attempt at having a minimal abstract API. Would I start with that platform in mind, surely 64 bit and usage of these specific functions for 64 bit I/O would be set.

    I went through all the trouble of providing 32 and 64 bit (actually, any bitness, like 128 bit for galactic storage, I like the abstraction) file offset support in the library just like the libc does. I won't throw that out again.

    But, I am open to adding explicit _64 API names in the header that are just dummies for the POSIX case (which defines the symbols at least as LFS wrappers already, if _LARGEFILE_BITS == 64 in the build) and actual i64 I/O API implementations for Windows in src/compat/. This needs messing with libmpg123's I/O code to redirect from the libc calls, either with more ifdefs in src/libmpg123/readers.c or a general redirection to compat routines and having central switches in libcompat only. I don't like either of those particularily, but I guess the extra redirection is better than to accumulate ifdefs for WIN32 in random places.

    In practice, I guess you can live with support for 32 bit file offsets especially for the use cases of a tracker library. 32 bit offsets are still plenty for instrument samples, eh?

    A compromise could be to just add some support for an explicit mpg123_replace_reader_handle_64 for Windows. Actually, this path could be used to hook in the 64 bit variants of the normal API in MSVC, like I do it with LFS wrappers already. Only issue is that libmpg123 still wouldn't use 64 bit offsets for things like the frame index.

    Here, I'd be open to something that defines internal use of off_t to int64_t, with fitting I/O use, while still having the case of native off_t as default. I know that 64 bit offsets is likely to be the only width we need for any forseeable future, but even consider the other way round: 32 bit is usually enough for compressed music, and there are still 32 bit microcontrollers/DSPs out there. People are known to build libmpg123 for such platforms where 64 bit numbers are no option. Been some years since he SHARC DSP port, but having a system-defined type for that helps.

    Oh, and the last one: About keeping filename encoding stuff outside the library. We do offer mpg123_open_handle() exactly for that. Or the feeder API. It's all there. The only issue is http://mpg123.org/api/group__mpg123__seek.shtml … the numbers that are passed to the replaced I/O routines.

     

    Last edit: Thomas Orgis 2021-06-07
    • manx

      manx - 2021-06-07

      Isn't that OK? It's more flexible, you can opt into the _64 names without configure now.

      Yes, I can make it work for me.

      In practice, I guess you can live with support for 32 bit file offsets especially for the use cases of a tracker library. 32 bit offsets are still plenty for instrument samples, eh?

      It's enough for OpenMPT and libopenmpt currently, as we, as you guessed, are limiting sample length to 32bit anyway.

      About the ssize_t thing. Would a #ifndef ssize_t #define ssize_t thing be more robust? I have no inclination to suffer too much pain because Microsoft decided that a signed size-like type does not make sense.

      Well, defining ssize_t (via typedef or #define) will break once MSVC changes it's opinion and starts to provide ssize_t. Or even when any other library decides to do what mpg123 does. Such problems tend to come up in projects that have a lot of dependencies. Staying away from defining commonly used names avoids such problems:

      #ifdef _MSC_VER
      typedef ptrdiff_t mpg123_ssize_t;
      #else
      typedef ssize_t mpg123_ssize_t;
      #endif
      

      and only using mpg123_ssize_t in the public header would address this (at the cost of breaking for anyone who assumed ssize_t to be available after including mpg123.h, which IMHO would be a good thing).

      In general, I would prefer to have libmpg123 not mess with any of the platform types at all. However, I do understand your concerns about maintaining backwards compatibility and understand that you do not intend to change that.

      A compromise could be to just add some support for an explicit mpg123_replace_reader_handle_64 for Windows. Actually, this path could be used to hook in the 64 bit variants of the normal API in MSVC, like I do it with LFS wrappers already. Only issue is that libmpg123 still wouldn't use 64 bit offsets for things like the frame index.
      Here, I'd be open to something that defines internal use of off_t to int64_t, with fitting I/O use, while still having the case of native off_t as default. I know that 64 bit offsets is likely to be the only width we need for any forseeable future

      Such a solution that allows for 64bit offsets by using replace_reader_handle_64() even when off_t is 32bit would be great.
      Regarding use of off_t as sample number or frame number, I think supporting 64bit is even more important here, especially for something like mpg123_tell(), which does overflow after roughly a month (if I did the math correctly) when playing a MP3 stream on Windows. (not relevant for OpenMPT, but for another project that I currently have in mind).

      but even consider the other way round: 32 bit is usually enough for compressed music, and there are still 32 bit microcontrollers/DSPs out there. People are known to build libmpg123 for such platforms where 64 bit numbers are no option.

      Understood.

       
  • Thomas Orgis

    Thomas Orgis - 2021-06-07

    I added the mpg123_ssize_t thing now. Sounds better than what was there before. Thanks.

    About internal 64 bit indices. This needs definition of an mpg123_off_t internally, I guess, that is normally set to off_t, but set to a 64 bit type if that type is available and off_t 32 bit.

    The 64 bit API could use int64_t explicitly, while libmpg123 could in theory still support even bigger offsets via off_t. A question is if a build with 32 bit internal type should offer the 64 bit API. Hm. Well, anyway … it's not something pressing that I'll invest time in now. I am swamped with other tasks.

    When things work now … you are not relyiing on mpg123 releases anyway, right, could just update from trunk? So could take some time scheduling 1.28.1. Maybe there's some other fallout to fix, too.

     
    • manx

      manx - 2021-06-07

      I added the mpg123_ssize_t thing now. Sounds better than what was there before.

      Thanks.

      When things work now … you are not relyiing on mpg123 releases anyway, right, could just update from trunk? So could take some time scheduling 1.28.1.

      Yes, works for me.
      I just merged r4942 and r4944 and adjusted our mpg123.h wrappers accordingly.

       
  • Evgeni Poberezhnikov

    @manx,

    Cmake solves precisely none of our requirements on a build system in OpenMPT and libopenmpt.

    You know, your buildsystem is not so cool as you think. Every time i update mpg123 Vcpkg port, i have problems with two ports. The fisrt is portaudio. The second is libopenmpt.

    And I suspect that if you have a bunch of build system projects in your build folder, you should definitely pay attention to CMake.

    Depending on system-specific and build-dependent types in the API and ABI is problematic in general (which is why the whole libmpg123 largefile macros exist at all).

    Here I agree with you.

     
    • manx

      manx - 2021-06-12

      You know, your buildsystem is not so cool as you think. Every time i update mpg123 Vcpkg port, i have problems with two ports. The fisrt is portaudio. The second is libopenmpt.

      This is SOLELY the problem of the vcpkg project. They never addressed my concerns with the libopenmpt port. See https://github.com/microsoft/vcpkg/pull/2669. Do your research before asserting things that I did not do.

      vcpkg is also unfit to be used as a package manager for our dependencies because it has no policy on providing security updates for the ports it includes in a timely manner (or even at all).

       
  • Evgeni Poberezhnikov

    This is SOLELY the problem of the vcpkg project. They never addressed my concerns with the libopenmpt port. See https://github.com/microsoft/vcpkg/pull/2669. Do your research before asserting things that I did not do.

    Yes, it looks like that. Well, then I beg your pardon.

     
    • manx

      manx - 2021-06-12

      Pardon granted. Apologies accepted. :)

       
  • Thomas Orgis

    Thomas Orgis - 2021-07-12
    • status: open --> closed-fixed
     

Log in to post a comment.