search

microsoft / vcpkg

C++ Library Manager for Windows, Linux, and MacOS
MIT License
22.9k stars 6.32k forks source link

libopenmpt port has been in abysmal shape from the start #18480

Closed manxorist closed 2 years ago

manxorist commented 3 years ago

@atkawa7 @alexkaratarakis @ras0219-msft @sagamusix

Even though I (libopenmpt upstream maintainer) have been initially positive about seeing libopenmpt packaged in vcpkg, I sadly have now changed my opinion after seeing this port completely unmaintained for 3 years. This is REALLY VERY BAD.

As time has shown, vcpkg is unable to properly maintain ports it contains. The port of libopenmpt included in vcpkg was broken from the start and did get merged in without any proper review. My comments on the original pull request (#2669) (which I only ever noticed after it was already merged) were never addressed. In particular, it has wrong dependencies, advertises a version number that breaks our SemVer versioning scheme, is a development and not a released version, contains multiple security issues by now [1] (as it never got updated), builds components it should not build, and other issues. Noone should use that port or should have used it, EVER.

The state of libopenmpt in vcpkg shines a very bad light on our project as well as on vcpkg itself. Seeing how bad our own software gets maintained as a vcpkg port, I will assume all other ports are of equally bad quality. Thus I will avoid to ever use vcpkg. To each and everyone asking, I will also strongly recommend to avoid vcpkg completely. vcpkg has proven itself to be unable or unwilling to deal with even basic software development best practices. vcpkg is shipping broken software. There is not even a recommended way to report security issues as far as I can see.

I am really frustrated here. You are packaging our software in a completely broken fashion, and do not even care to fix it.

Please either do your absolutely basic duties of at least maintaining the libopenmpt port properly, or just remove it completely.

[1]

alexkaratarakis commented 3 years ago

Hi @manxorist ,

Given the issues raised on your library's vcpkg port, I think it's reasonable to remove the libopenmpt port until a satisfactory port is submitted. The current port (along with the comments in #2669) would be a great starting point for such a port, but it will still be available through git even after removal in the main branch.

For now, I have submitted a PR with the removal.

JackBoosY commented 3 years ago

Sorry for that! We always hope that the owner or maintainers of the port will deal with the issues of the port in vcpkg, because they know the relevant informations better than us. And put the contact information in the field maintainer of CONTROL or manifest. However, this is rarely done so far, leading to similar situations. We are deeply sorry for this.

Neumann-A commented 3 years ago

Hmm can the port not simply be update and switched to vcpkg_configure_make. The stuff in https://github.com/OpenMPT/openmpt/tree/master/build/autotools looks like it should be the main buildsystem. The configure.ac looks like it is properly searching for dependencies so using that should be easy to setup. Maybe vcpkg_configure_make needs to be extended for out-of-tree configure.ac files

manxorist commented 3 years ago

Hmm can the port not simply be update and switched to vcpkg_configure_make. The stuff in https://github.com/OpenMPT/openmpt/tree/master/build/autotools looks like it should be the main buildsystem. The configure.ac looks like it is properly searching for dependencies so using that should be easy to setup. Maybe vcpkg_configure_make needs to be extended for out-of-tree configure.ac files

No. The correct thing to do would be to update the vcpkg cmake build system with the comments I made in the pull request. We do not support Autotools for MSVC builds. If vcpkg wants to provide its own build system (which there are totally valid reasons for, see discussion in pull request), it should do so by using its own preferred build system, instead of redirecting to something that is not supported for the task at hand. Using Autotools for MSVC would lead to yet another state of unmaintainability and quirks proprietary to vcpkg.

We (libopenmpt) do not have a preferred fit-for-all build system. Instead, we try to use build systems that are native to the respective target platforms (i.e. Autotools for Linux/BSD/Homebrew/Unix-like, msbuild/VisualStudio for Windows, ndk-build for Android, and a custom Makefile for somewhat special platforms (DJGPP, Emscripten, MinGW, ...). As vcpkg tries to support multiple platforms, adapting to each of our build system for each individual platform does not seem desirable or scalabe to me, A portable cmake build system that interoperates properly with the vcpkg infrastructure would be a better fit, IMHO. As libopenmpt already supports various build system, the code base is geared towards not requiring special features of the build system, and thus it should be rather simple to provide another build system for vcpkg.

manxorist commented 3 years ago

For now, I have submitted a PR with the removal.

Thanks, though I would still prefer is someone could just implement the changes I suggested and use a proper released version.

Neumann-A commented 3 years ago

No. The correct thing to do would be to update the vcpkg cmake build system with the comments I made in the pull request. We do not support Autotools for MSVC builds. If vcpkg wants to provide its own build system (which there are totally valid reasons for, see discussion in pull request), it should do so by using its own preferred build system, instead of redirecting to something that is not supported for the task at hand. Using Autotools for MSVC would lead to yet another state of unmaintainability and quirks proprietary to vcpkg.

I think you have a pretty damn buildsystem mess but that might be just my opinion. Autotools is already an abstraction and if you cannot support a native windows build with <arch>-pc-mingw32 and using compile cl.exe as an compiler then something is wrong with your build scripts.

As vcpkg tries to support multiple platforms, adapting to each of our build system for each individual platform does not seem desirable or scalabe to me, A portable cmake build system that interoperates properly with the vcpkg infrastructure would be a better fit, IMHO.

Now I am ok to remove the port since it is not vcpkgs responsibility to maintain its own buildsystem for a port. It simply doesn't scale to vendor a CMakeLists.txt and makes updates to a port extremely difficult and time consuming. vcpkg should just invoke the upstream buildsystem and be done with it is my philosophy. Even if it means using vcpkg_configure_msbuild which currently doesn't pass custom compiler flags.

manxorist commented 3 years ago

No. The correct thing to do would be to update the vcpkg cmake build system with the comments I made in the pull request. We do not support Autotools for MSVC builds. If vcpkg wants to provide its own build system (which there are totally valid reasons for, see discussion in pull request), it should do so by using its own preferred build system, instead of redirecting to something that is not supported for the task at hand. Using Autotools for MSVC would lead to yet another state of unmaintainability and quirks proprietary to vcpkg.

I think you have a pretty damn buildsystem mess but that might be just my opinion. Autotools is already an abstraction and if you cannot support a native windows build with <arch>-pc-mingw32 and using compile cl.exe as an compiler then something is wrong with your build scripts.

I just do not have the time to test this nonsense which nobody except vcpkg would be using anyway. Supporting autotools on Windows with MSVC requires the user to install at least 2 build infrastructures in the first place. This would be an abomination of a build system, and solves no real world problem. To the contrary, it even introduces build system requirements alien to the platform at hand.

As vcpkg tries to support multiple platforms, adapting to each of our build system for each individual platform does not seem desirable or scalable to me, A portable cmake build system that interoperates properly with the vcpkg infrastructure would be a better fit, IMHO.

Now I am ok to remove the port since it is not vcpkgs responsibility to maintain its own buildsystem for a port. It simply doesn't scale to vendor a CMakeLists.txt and makes updates to a port extremely difficult and time consuming. vcpkg should just invoke the upstream buildsystem and be done with it is my philosophy. Even if it means using vcpkg_configure_msbuild which currently doesn't pass custom compiler flags.

Your argument is false, as outlined by @ras0219-msft in the pull request.

So it is vcpkg's responsibility to blackmail all ports into providing a cmake buildsystem upstream in order to be properly supported by vcpkg? This scales wonderfully for vcpkg, but insults everyone else. Great attitude.

Neumann-A commented 3 years ago

I just do not have the time to test this nonsense which nobody except vcpkg would be using anyway. Supporting autotools on Windows with MSVC requires the user to install at least 2 build infrastructures in the first place. This would be an abomination of a build system, and solves no real world problem. To the contrary, it even introduces build system requirements alien to the platform at hand.

Autotools is just a bunch of scripts. Nothing more. Every script needs some kind of interpreter. The only reason to say it is not native is that bash and probably pkg-config are not "common" (whatever that means) tools on windows. vcpkg provides all the tools to make such a build successful.

Your argument is false, as outlined by @ras0219-msft in the pull request.

Link to comment?

So it is vcpkg's responsibility to blackmail all ports into providing a cmake buildsystem upstream in order to be properly supported by vcpkg? This scales wonderfully for vcpkg, but insults everyone else. Great attitude.

Nobody is blackmailing anybody. vcpkg can work with (cmake|meson|autotools|make|msbuild|qmake|gn|b2) and even strange perl scripts (libpq [could maybe be switched to autotools?]). The only thing I am saying is that vcpkg should not try vendor a CMakeLists.txt only because it is difficult to correctly invoke the native upstream buildsystem.

manxorist commented 3 years ago

Your argument is false, as outlined by @ras0219-msft in the pull request.

Link to comment?

Implied by https://github.com/microsoft/vcpkg/pull/2669#discussion_r164761311 and suggesting how to proceed: https://github.com/microsoft/vcpkg/pull/2669#discussion_r164903514

manxorist commented 3 years ago

So it is vcpkg's responsibility to blackmail all ports into providing a cmake buildsystem upstream in order to be properly supported by vcpkg? This scales wonderfully for vcpkg, but insults everyone else. Great attitude.

Nobody is blackmailing anybody. vcpkg can work with (cmake|meson|autotools|make|msbuild|qmake|gn|b2) and even strange perl scripts (libpq [could maybe be switched to autotools?]). The only thing I am saying is that vcpkg should not try vendor a CMakeLists.txt only because it is difficult to correctly invoke the native upstream buildsystem.

However it cannot do so properly:

Even if it means using vcpkg_configure_msbuild which currently doesn't pass custom compiler flags.

manxorist commented 3 years ago

I just do not have the time to test this nonsense which nobody except vcpkg would be using anyway. Supporting autotools on Windows with MSVC requires the user to install at least 2 build infrastructures in the first place. This would be an abomination of a build system, and solves no real world problem. To the contrary, it even introduces build system requirements alien to the platform at hand.

Autotools is just a bunch of scripts. Nothing more. Every script needs some kind of interpreter. The only reason to say it is not native is that bash and probably pkg-config are not "common" (whatever that means) tools on windows. vcpkg provides all the tools to make such a build successful.

And again you are demanding upstream to provide a solution that is only viable in the context of vcpkg.

Neumann-A commented 3 years ago

However it cannot do so properly: Even if it means using vcpkg_configure_msbuild which currently doesn't pass custom compiler flags.

That is a known issue. It has simply not yet been fixed because there are not that many msbuild ports which is why I proposed vcpkg_configure_make since your configure scripts looked like they would be able to handle that. (Because it also looks quite similar to what a CMakeLists.txt would look like for it.)

Implied by #2669 (comment) and suggesting how to proceed: #2669 (comment)

This seems more and more like a request to provide a CMakeLists.txt to the project.

manxorist commented 3 years ago

Implied by #2669 (comment) and suggesting how to proceed: #2669 (comment)

This seems more and more like a request to provide a CMakeLists.txt to the project.

So we are back at vcpkg requesting upstream to implement vcpkg-specific solutions to be supported properly. You are arguing in circles here.

BillyONeal commented 3 years ago

vcpkg absolutely does not require that the build system be cmake, but it does require that the results are consistently consumable from cmake projects on different platforms; that's vcpkg's whole raison d'être. We patch upstream builds to fix this regularly, most often to make sure libs are placed in the right locations.

That doesn't mean we need a new cmake build system, but it may be difficult to achieve when driving the process with several different build systems on different platforms.

alexkaratarakis commented 3 years ago

Thanks, though I would still prefer is someone could just implement the changes I suggested and use a proper released version.

I agree, but there are still open questions about what the port should look like so the fixes can be more comfortably done in a new PR that can incubate for sufficient amount of time. The goal here is for someone to provide a port of satisfactory quality so libopenmpt maintainers and libopenmpt users_that_use_vcpkg are happy.

manxorist commented 3 years ago

vcpkg absolutely does not require that the build system be cmake, but it does require that the results are consistently consumable from cmake projects on different platforms; that's vcpkg's whole raison d'être.

Well, that might be the goal, however I highly doubt that consistent results are achievable reliably with different build systems on different platforms, which is why (after this was explained to me in the original pull request), I agreed that cmake would probably be a better fit to support all platforms that vcpkg wants to support in an as consistent as possible fashion.

I even suggested that I would take a contribution of a cmake build system (if it does not depend on vcpkg) upstream. See https://github.com/microsoft/vcpkg/pull/2669#discussion_r164781796.

I still somewhat doubt that cmake would have any benefits outside of vcpkg for libopenmpt upstream, as to me, cmake feels alien on EVERY platform.

Neumann-A commented 3 years ago

I still somewhat doubt that cmake would have any benefits outside of vcpkg for libopenmpt upstream, as to me, cmake feels alien on EVERY platform.

If you don't want to use CMake use meson..... CMake is not alien to any platform... maybe you think so because it provides an abstraction about the build/target platform. On the other hand you are already using premake to generate the VS solutions (How is premake different from cmake in this case?), so why not simply switch that correctly to cmake and also include the unix/osx case? Or make the autotool build correctly support the windows case (I wonder isn't this a problem for mingw builds or are those totally un(der)supported?)....... I think the problem is that libopenmpts build system is fragmented. The premake lua scripts are only for windows while your autotools scripts are only for unix (I assume so since the PluginBridge build seems to be missing).

manxorist commented 3 years ago

I still somewhat doubt that cmake would have any benefits outside of vcpkg for libopenmpt upstream, as to me, cmake feels alien on EVERY platform.

CMake is not alien to any platform...

cmake is not the native build system on any platform. Thus it is alien on all of them. Cmake integration with pkg-config (using dependencies from and/or providing dependencies for pkg-config style projects) is still awkward and frowned upon by cmake, and cmake still tries to enforce its own way of package discovery (which is proprietary to cmake and alien to any platform). Now, I do understand the push of vcpkg to use cmake and cmake's dependency detection functionality because of its consistency on all platforms, however this still does not change the fact that it is proprietary to cmake and not the native way on all of them. vcpkg seriously needs to correct its communication here. vcpkg requires cmake to function correctly cross-platform, and it should communicate so clearly.

On the other hand you are already using premake to generate the VS solutions (How is premake different from cmake in this case?), so why not simply switch that correctly to cmake and also include the unix/osx case?

Just tried again with the latest cmake version and had a good laugh: The Visual Studio project files it generates are still completely weird, use tons of custom shell scripts internally, USE ABSOLUTE PATHNAMES, and other idiotic things. Premake does none of such nonsense. The Visual Studio project files generated by cmake are not fit for committing to a repository. I understand that this is intended by cmake, however this fact alone breaks basic usability:

  1. install native development tools (Visual Studio)
  2. checkout source tree
  3. open native project file (solution file)
  4. build
  5. run

Having a properly native feeling experience in Visual Studio is crucial for developing OpenMPT and libopenmpt. Neither cmake itself nor the Visual Studio cmake integration can achieve that. Premake generated project files can.

Or make the autotool build correctly support the windows case (I wonder isn't this a problem for mingw builds or are those totally un(der)supported?).......

Again, requiring to install 2 build systems is nonsense and will not be a supported configuration. MinGW/msys is supported with Autotools (in particular also cross-compiling from Linux to MinGW).

I think the problem is that libopenmpts build system is fragmented.

Fact, but not a problem in itself. This has the major advantage of allowing each platform to use its native and preferred build system. I understand that this conflicts with package managers that try to cross the platform boundary, and precisely for this reason I will still take a cmake build system as a contribution.

The premake lua scripts are only for windows while your autotools scripts are only for unix (I assume so since the PluginBridge build seems to be missing).

PluginBridge has nothing to do with libopenmpt. It is only used by OpenMPT (the Windows GUI application).

Neumann-A commented 3 years ago

cmake is not the native build system on any platform. Thus it is alien on all of them. Cmake integration with pkg-config (using dependencies from and/or providing dependencies for pkg-config style projects) is still awkward and frowned upon by cmake, and cmake still tries to enforce its own way of package discovery (which is proprietary to cmake and alien to any platform).

awkward and frowned upon by cmake

I don't think so. FindPkgConfig is totally fine and works.

cmake still tries to enforce its own way of package discovery

It doesn't force you to use it...... If you mean the find_package approach which is just a redirection to/include of a Find<Module>.cmake or a <Module>-Config.cmake. This is just proper modularization of the build script. If you don't want to use it don't use it and do whatever scripting you want .... You can have Fetch_Content logic in the Find<Module>.cmake or just do find_library or use FindPkgConfig in it (or all of those approaches).

alien to any platform vcpkg requires cmake to function correctly cross-platform, and it should communicate so clearly.

to run vcpkg it requires cmake because the scripts are written in CMake but a port/library does not require to be CMake. vcpkg itself downloads cmake if required. As already said vcpkg has support for numerous build system....the portfile.cmake is just a script invoking whatever buildsystem depending on what was contributed to said port.

USE ABSOLUTE PATHNAMES, and other idiotic things.

Generated solutions are not intended for distribution. MSBuild might have to re-invoke CMake to regenerate.....

Again, requiring to install 2 build systems is nonsense and will not be a supported configuration. MinGW/msys is supported with Autotools (in particular also cross-compiling from Linux to MinGW).

Doesn't matter vcpkg takes care of it. If it is so that libopenmpt supports MinGW then it can easily be packaged in vcpkg using vcpkg_configure_make for all platforms 📦 even if you don't like the approach it will work and probably also generate working *.pc files for users to consume.... (which are probably alien for you on native windows but vcpkg deals with that alienation for you)

So but my question would be: What is the native/not alien way to consume libopenmpt on Windows?

manxorist commented 3 years ago

Generated solutions are not intended for distribution.

So cmake fails my requirements. Why should I use it then?

Again, requiring to install 2 build systems is nonsense and will not be a supported configuration. MinGW/msys is supported with Autotools (in particular also cross-compiling from Linux to MinGW).

Doesn't matter vcpkg takes care of it. If it is so that libopenmpt supports MinGW then it can easily be packaged in vcpkg using vcpkg_configure_make for all platforms 📦 even if you don't like the approach it will work and probably also generate working *.pc files for users to consume.... (which are probably alien for you on native windows but vcpkg deals with that alienation for you)

It matters because vcpkg would be using a configuration unsupported by upstream.

So but my question would be: What is the native/not alien way to consume libopenmpt on Windows?

Like any other project on Windows when not using any of the package managers. Either include and link by hand, or reference the msbuild file.

Unless package managers start to provide security updates for all ports they distribute in a timely fashion (and vcpkg has just proven itself incapable of doing so), this is also the only viable way to responsibly use any library on Windows.

manxorist commented 3 years ago

cmake is not the native build system on any platform. Thus it is alien on all of them. Cmake integration with pkg-config (using dependencies from and/or providing dependencies for pkg-config style projects) is still awkward and frowned upon by cmake, and cmake still tries to enforce its own way of package discovery (which is proprietary to cmake and alien to any platform).

awkward and frowned upon by cmake

I don't think so. FindPkgConfig is totally fine and works.

cmake still tries to enforce its own way of package discovery

It doesn't force you to use it...... If you mean the find_package approach which is just a redirection to/include of a Find<Module>.cmake or a <Module>-Config.cmake. This is just proper modularization of the build script. If you don't want to use it don't use it and do whatever scripting you want .... You can have Fetch_Content logic in the Find<Module>.cmake or just do find_library or use FindPkgConfig in it (or all of those approaches).

My information is based on a talk by some either CMake developer or at least proponent who acknowledged bad integration with pkg-config in various aspects. I do not remember precise details, but if really needed I can try to dig up the talk in question (might have been a CppCon talk).

Even ignoring that, unless CMake natively without manual intervention generates the .pc files itself, its integration with pkg-config is insufficient in my book. It needs to be agnostic of which method it uses to find dependencies, and which it uses to make the current project findable. It actually does not solve the problem it wants to solve here, if I have to dig out myself if a package in question is using CMake method of finding or if it's using the platform-native way.

Neumann-A commented 3 years ago

So cmake fails my requirements. Why should I use it then?

You haven't even defined your requirements..... But I can already say that your generated solutions don't even fulfill my build requirements and would require manual adjustment anyway.

It matters because vcpkg would be using a configuration unsupported by upstream.

It might be untested but it will work.... It is not unsupported because you are testing MinGW and switching to CL is just switching out the backend compiler (e.g. like switching from g++ to clang++).

Like any other project on Windows when not using any of the package managers. Either include and link by hand, or reference the msbuild file.

ah yes..... I already see people suffering with that approach.....

Unless package managers start to provide security updates for all ports they distribute in a timely fashion (and vcpkg has just proven itself incapable of doing so), this is also the only viable way to responsibly use any library on Windows.

vcpkg needs contributors. If the library maintainer is not updating than some user has to do the work. Don't forget: vcpkg is not a service. vcpkg is a open source project which requires contributors. If you don't like the port of libopenmpt than contribute. If you don't like the CLA that is up to you but be aware that "code" needs to reach a certain "value|height" to be protect-able at all. Writing simple portfiles probably doesn't fall into this category at all.

My information is based .....

FindPkgConfig works in single configuration builds. The bad integration is probably in multi config builds.... but that doesn't matter for vcpkg because in vcpkg everything is build as a single configuration.

Even ignoring that, unless CMake natively without manual intervention generates the .pc files itself, its integration with pkg-config is insufficient in my book.

Then go use meson instead..... meson can automatically generate .pc files.

It needs to be agnostic of which method it uses to find dependencies, and which it uses to make the current project findable

That is what find_package is for. But it is your responsibility to write the Find<Module>.cmake in an agnostic way....

manxorist commented 3 years ago

So cmake fails my requirements. Why should I use it then?

You haven't even defined your requirements.....

This is ridiculous, I very much did:

The Visual Studio project files generated by cmake are not fit for committing to a repository. I understand that this is intended by cmake, however this fact alone breaks basic usability:

  1. install native development tools (Visual Studio)
  2. checkout source tree
  3. open native project file (solution file)
  4. build
  5. run

Having a properly native feeling experience in Visual Studio is crucial for developing OpenMPT and libopenmpt. Neither cmake itself nor the Visual Studio cmake integration can achieve that. Premake generated project files can.

You are free to ignore my arguments or assess that you personally do not care about them, but neither will invalidate them.

I have seen WAY TOO MANY projects that fail this very basic test. Now, I do understand that vcpkg's intention is to solve this problem, but it still needs to go a very long way to even be close.

But I can already say that your generated solutions don't even fulfill my build requirements and would require manual adjustment anyway.

So ... you could ... like ... contribute? ;) We do not even require signing unacceptable CLAs.

Like any other project on Windows when not using any of the package managers. Either include and link by hand, or reference the msbuild file.

ah yes..... I already see people suffering with that approach.....

True, but there is no established alternative. Requiring reliable security policies is a very basic requirement. Unless vcpkg does that, I will not consider it viable in any way.

Unless package managers start to provide security updates for all ports they distribute in a timely fashion (and vcpkg has just proven itself incapable of doing so), this is also the only viable way to responsibly use any library on Windows.

vcpkg needs contributors. If the library maintainer is not updating than some user has to do the work. Don't forget: vcpkg is not a service. vcpkg is a open source project which requires contributors. If you don't like the port of libopenmpt than contribute. If you don't like the CLA that is up to you but be aware that "code" needs to reach a certain "value|height" to be protect-able at all. Writing simple portfiles probably doesn't fall into this category at all.

Your documentation disagrees: (https://github.com/microsoft/vcpkg/blob/master/CONTRIBUTING.md)

You will need to complete a Contributor License Agreement (CLA) before your pull request can be accepted.

It does not specify that this would not be required for ports.

I cannot contribute to vcpkg. vcpkg is sadly not what I would consider "open".

It needs to be agnostic of which method it uses to find dependencies, and which it uses to make the current project findable

That is what find_package is for. But it is your responsibility to write the Find<Module>.cmake in an agnostic way....

So, either each package that wants to be findable by cmake or that wants to find a non-cmake package needs to provide this. This is precisely what I meant by "cmake is trying to enforce its own way".

manxorist commented 3 years ago

To reiterate on the CLA problem:

I fully understand and support the CLA requirement for contributions to the vcpkg tool and infrastructure itself. I can totally see that Microsoft may intend to ship that as part of Visual Studio or Windows or some SDK and license it to paying customers under whatever conditions both parties see fit. I can understand that Microsoft does not want to be bound by even the liberal MIT license here. I can also see the tradeoff of the risk of alienating possible contributors to be in favor of enforcing the CLA.

However, for ports this is frankly just silly. I know of multiple people (who would have been perfectly capable of helping me solving the libopenmpt vcpkg port issue long in the past) who refused to sign the CLA and thus felt unwelcomed to contribute to a vcpkg port.

The solution is pretty darn simple here: Split the ports directory out of the main vcpkg repository and remove the CLA requirement from the ports repository. Microsoft would not need the CLA for anything if the vcpkg repository would just link to the live online ports repository on Github.

atkawa7 commented 3 years ago

vcpkg needs contributors. If the library maintainer is not updating than some user has to do the work. Don't forget: vcpkg is not a service. vcpkg is a open source project which requires contributors. If you don't like the port of libopenmpt than contribute

+1 for this

Neumann-A commented 3 years ago

However, for ports this is frankly just silly. I know of multiple people (who would have been perfectly capable of helping me solving the libopenmpt vcpkg port issue long in the past) who refused to sign the CLA and thus felt unwelcomed to contribute to a vcpkg port. The solution is pretty darn simple here: Split the ports directory out of the main vcpkg repository and remove the CLA requirement from the ports repository. Microsoft would not need the CLA for anything if the vcpkg repository would just link to the live online ports repository on Github.

Provide your own overlay-port or ports registry..... https://github.com/microsoft/vcpkg/blob/master/docs/specifications/ports-overlay.md https://github.com/microsoft/vcpkg/blob/master/docs/specifications/registries.md I mean I have such stuff for my private repos..... but be aware that being out of the main repo means no automatic CI tests for you....

I have seen WAY TOO MANY projects that fail this very basic test.

We have different views on what is native/alien. For me as a VS Code user or VS open folder user I simply don't care to differentiate between your "native/alien". it just tools. Either you can get them easily or you cannot get them easily. Newer VS version install cmake by default? So cmake is native on windows now? CMake can be obtained easily on all platforms and if i want to debug using the VS IDE I just use cmake with the VS Generator and open up the solution for debugging. I can edit the sources in the IDE without any problems etc.

Your documentation disagrees: (https://github.com/microsoft/vcpkg/blob/master/CONTRIBUTING.md)

no it doesn't. Bigger companies just want to have 100% legal certainty instead of fighting unnecessary lawsuits.

We do not even require signing unacceptable CLAs.

You do and you already "signed" one...... I find it funny how people tend to forget it: https://docs.github.com/en/github/site-policy/github-terms-of-service and the CLA is not much different from that except replacing GitHub with Microsoft and missing the explanatory part of it. (Also Github is basically Microsoft if you didn't know that: https://news.microsoft.com/announcement/microsoft-acquires-github/)

I cannot contribute to vcpkg. vcpkg is sadly not what I would consider "open".

It is. Since this repository is on Github and public. So Microsoft itself has to follow https://docs.github.com/en/github/site-policy/github-terms-of-service

So, either each package that wants to be findable by cmake or that wants to find a non-cmake package needs to provide this. This is precisely what I meant by "cmake is trying to enforce its own way".

You are too much locked into your view of things..... CMake is providing an abstraction.... nothing more. If you don't want to use it: include(FindPkgConfig) instead of find_package(PkgConfig) + an additional pkg_check_modules(WHATEVER) which is basically the same as autotools PKG_CHECK_MODULES

atkawa7 commented 3 years ago

You are too much locked into your view of things.....

Couldn't have said it better

manxorist commented 3 years ago

We do not even require signing unacceptable CLAs. You do

Stop lying. We very much do not. We are not even using Github as our primary repository.

manxorist commented 3 years ago

The GitHub Terms of Service are not even remotely similar to the terms in the Microsoft Contributor License Agreement. This claim is so obviously wrong, that I will not discuss anything any further with @Neumann-A as they are obviously not interested in any discussion based on actual facts.

manxorist commented 3 years ago

Back to technical discussion.

I am trying to assess the implications of supporting CMake as a build system in libopenmpt upstream.

When libopenmpt starts to support CMake as an official build system, this will involve providing the files necessary for other CMake projects to be able to use find_package. Now, if Linux distributions do not switch to the CMake build system of libopenmpt, they will continue to use Autotools and pkg-config. Now, if some other project uses CMake and depends on libopenmpt, the Autotools build system must now also provide whatever is necessary for CMake to find libopenmpt, right?

ras0219-msft commented 3 years ago

I humbly request that further responses to this thread are restricted to just @manxorist or Microsoft Employees. Otherwise, I'll need to lock this thread and we can move this important discussion either to a new thread or email.

First, I want to sincerely apologize that your comments on the original Pull Request for libopenmpt were left unheard; our review process doesn't cover already-merged items. I'll look into how we can update them to avoid this happening for anyone else.

Second, thank you very much for building and maintaining libopenmpt -- open source authorship and maintenance is universally underappreciated, so let me explicitly thank you for the years of effort that have gone into it so far!

Next, I'm sorry that the MSFTCLA is a blocker for you to contribute. This is intended to ensure we can continue to distribute the files submitted to this repo forever (such as the recipes and patches); we're absolutely not trying to sneakily steal ownership of your original library. Unfortunately, this is a place where my hands are tied -- all Microsoft repositories are covered by the CLA, so splitting off the ports tree would not change this situation (it would still be a Microsoft repository). If it would help at all, I'd be more than happy to get you in touch with a member of our legal team.

If despite the poor experience you've had so far you'd like to support adoption of libopenmpt through package managers like vcpkg, we now have the ability to support third party registries. This means you could host the recipe of libopenmpt yourself without signing any CLAs and have full control to keep it up to date with the releases. If this is interesting to you, I'd be happy to go into more details.

As said above by @BillyONeal, our objective is to enable developers to easily write cross-platform applications, using unified tooling. This means that it's very important that users can consume libraries in a unified way; either via CMake -config files, or pkg-config, or some other means. Usually, we find it easiest to implement this by using the same buildsystem on all platforms, but we do have precedence for using different buildsystems per target. We prefer to avoid it because this is almost always worse in one or many ways: authors don't ensure options are consistent across the different options (usually MSBuild doesn't offer any options!) or the integration piece (.cmake or .pc) is only available in one place.

I'm very happy about your suggestion to contribute a CMakeLists.txt to upstream (mentioned both in this thread and in some of the links) -- I intend to take your feedback from the original PR into account and make a PR which will hopefully satisfy everyone involved.

Once again, I'm very sorry about not meeting your reasonable expectations so far and I sincerely intend to do better moving forward.

Edit: I was typing this response while the previous message came in, so this doesn't cover that yet :)

ras0219-msft commented 3 years ago

Now, if some other project uses CMake and depends on libopenmpt, the Autotools build system must now also provide whatever is necessary for CMake to find libopenmpt, right?

A perfect 10/10 library would provide the same integration from each buildsystem, yes. However, the majority of libraries with multiple buildsystems unfortunately don't do this. If they offer CMake and autotools, the autotools build almost universally will not provide CMake integration. Fortunately the reverse is reasonably common (CMake builds providing .pc files)

This is a shame, because it's actually quite easy to do. Essentially, CMake's integration works via translating a find_package(XYZ) call into loading XYZ-config.cmake. Therefore, to provide CMake integration, you can simply use something like:

# libopenmpt-config.cmake
get_filename_component(_IMPORT_PREFIX "${CMAKE_CURRENT_LIST_FILE}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
if(_IMPORT_PREFIX STREQUAL "/")
  set(_IMPORT_PREFIX "")
endif()

add_library(libopenmpt STATIC IMPORTED)
set_target_properties(libopenmpt PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
)

get_filename_component(_DIR "${CMAKE_CURRENT_LIST_FILE}" PATH)
file(GLOB CONFIG_FILES "${_DIR}/libopenmpt-targets-*.cmake")
foreach(f ${CONFIG_FILES})
  include(${f})
endforeach()
# libopenmpt-targets-release.cmake
set_property(TARGET libopenmpt APPEND PROPERTY IMPORTED_CONFIGURATIONS RELEASE)
set_target_properties(libopenmpt PROPERTIES
  IMPORTED_LOCATION_RELEASE "${_IMPORT_PREFIX}/lib/libopenmpt.a"
  )

This is a rough sketch that I haven't tested, but the general idea is to create an IMPORTED CMake target that represents the original library. The "cute" glob-load enables better support for having release and debug versions side-by-side (which vcpkg does provide to ensure MSBuild-users have a native-feeling experience).

manxorist commented 3 years ago

I humbly request that further responses to this thread are restricted to just @manxorist or Microsoft Employees. Otherwise, I'll need to lock this thread and we can move this important discussion either to a new thread or email.

Thank you very very much for intervening here and for taking this discussion back on a more constructive path. Much appreciated!

First, I want to sincerely apologize that your comments on the original Pull Request for libopenmpt were left unheard;

Thanks!

our review process doesn't cover already-merged items. I'll look into how we can update them to avoid this happening for anyone else.

Well, having comments on an already merged or already closed issues forgotten is only one side of the issue here. I guess in that regard, re-opening the issue would probably have helped to keep track. As I said in https://github.com/microsoft/vcpkg/issues/503#issuecomment-361599406, I had subscribed to the port request issue, but I was not notified by GitHub about the pull request. I am not aware of any GitHub configuration that I could change to receive notifications in such situations. This looks somewhat like a GitHub usability issue to me. Maybe it is even fixed already - I have not been in a similar situation for a long time, as far as I can remember, thus I cannot tell.

Another suggestion that could have avoided most of the trouble would be, that you could start to @-mention the top N maintainers of upstream projects (if they have associated GitHub accounts) in a pull request discussion for a new port before merging it. I would assume maintainers of any project could provide useful insight about how their package could be included best in package managers. Even if they choose to not review the port, it would still make them aware of the vcpkg port, and also promote vcpkg itself.

Second, thank you very much for building and maintaining libopenmpt -- open source authorship and maintenance is universally underappreciated, so let me explicitly thank you for the years of effort that have gone into it so far!

Thanks, we should also thank the original author of ModPlug Tracker and libmodplug, as well as all other former and current OpenMPT developers.

I would also like to thank you and all other contributors for working on vcpkg. Even though I am not using it myself, and even though I in some aspects disagree with some of the design choices, I do support its objectives.

Next, I'm sorry that the MSFTCLA is a blocker for you to contribute. This is intended to ensure we can continue to distribute the files submitted to this repo forever (such as the recipes and patches); we're absolutely not trying to sneakily steal ownership of your original library. Unfortunately, this is a place where my hands are tied -- all Microsoft repositories are covered by the CLA, so splitting off the ports tree would not change this situation (it would still be a Microsoft repository). If it would help at all, I'd be more than happy to get you in touch with a member of our legal team.

As there appears to be little hope of changing either Microsoft's or my opinion on that matter, I would rather prefer to work towards solving the technical issues.

If despite the poor experience you've had so far you'd like to support adoption of libopenmpt through package managers like vcpkg, we now have the ability to support third party registries. This means you could host the recipe of libopenmpt yourself without signing any CLAs and have full control to keep it up to date with the releases. If this is interesting to you, I'd be happy to go into more details.

I am aware of the possibility to provide a separate registry for vcpkg (I learned about that in some cppcast episode, IIRC). As I am not not a user of vcpkg myself, I currently have little intention to work on something like that though. To me, it feels like not being included in the official vcpkg repository would make libopenmpt somewhat of a second-class citizen in vcpkg.

I do still prefer a proper port of libopenmpt in vcpkg, as both vcpkg's users as well as libopenmpt's users would benefit.

As said above by @BillyONeal, our objective is to enable developers to easily write cross-platform applications, using unified tooling. This means that it's very important that users can consume libraries in a unified way; either via CMake -config files, or pkg-config, or some other means. Usually, we find it easiest to implement this by using the same buildsystem on all platforms, but we do have precedence for using different buildsystems per target. We prefer to avoid it because this is almost always worse in one or many ways: authors don't ensure options are consistent across the different options (usually MSBuild doesn't offer any options!) or the integration piece (.cmake or .pc) is only available in one place.

Very true, and libopenmpt is guilty of that as well. However, often there are options that make little sense in the context of some particular buildsystem or platform. Keeping things in the respective buildsystems as tidy as possible simplifies matters for users who are not using a cross-platform package manager on any particular platform. I fully understand though that in the context of a multi-platform package manager like vcpkg (or conan), a multi-platform buildsystem is strongly preferred.

I'm very happy about your suggestion to contribute a CMakeLists.txt to upstream (mentioned both in this thread and in some of the links) -- I intend to take your feedback from the original PR into account and make a PR which will hopefully satisfy everyone involved.

Great, thanks!

Once again, I'm very sorry about not meeting your reasonable expectations so far and I sincerely intend to do better moving forward.

Thanks again! :)

manxorist commented 3 years ago

Therefore, to provide CMake integration, you can simply use something like:

So far, this looks reasonably manageable, however, would this still be as easy to do when we consider the libopenmpt dependency graph? libopenmpt depends on zlib, ogg, vorbis, vorbisfile, and mpg123:

libopenmpt -> zlib
libopenmpt -> mpg123
libopenmpt -> ogg
libopenmpt -> vorbis
libopenmpt -> vorbisfile
vorbisfile -> ogg
vorbisfile -> vorbis

Each of these libraries is buildable by both, Autotools and CMake. A quick search on my Ubuntu 20.04 install shows that none of these libraries provide a FOO-config.cmake there (for zlib there is a FindZLIB.cmake which appears to be provided by cmake itself?!). So, if we want our Autotools buildsystem to support find_package from a CMake project, we would now have to deduce from within Autotools (without having CMake even available at build time) whether our dependencies are findable by CMake via find_package or via pkg-config, in order to properly list the transitive/private dependencies in libopenmpt-config.cmake, right?

I am still trying to understand what would be the best common practices here. It is perfectly possible that I might be missing something crucial, though.

ras0219-msft commented 3 years ago

Sorry for the long time to get back!

First, I've opened #18933 which I believe addresses all the comments you made on the initial PR, along with a bunch of other "goodness" changes. Once you're happy with the CMakeLists.txt there, I'll open a PR into the upstream libopenmpt repo (CMakeLists.txt into the root?).

Ubuntu missing the config files for your dependencies is definitely an issue; when built with CMake, vorbis does provide them (https://github.com/xiph/vorbis/blob/4e1155cc77a2c672f3dd18f9a32dbf1404693289/lib/CMakeLists.txt#L134-L137) but I suspect they won't provide them when built with autotools. I suspect a similar story for the other libraries.

I think there are two reasonable approaches that are maintainable:

  1. Assume CMake users will have CMake configs available. For users that want to build using specifically the apt libraries, they can use autoconf which is entirely consistent from that environment. From your CMake build, you would publish a CMake config and optionally pc files (this is the current implementation I'm proposing). This is the most cross-platform option in my opinion.
  2. Only deal in .pc files. From the CMake build, use the pkg-config helpers to consume from .pc files and only publish .pc files. This is a bit unfortunate because it forces anyone downstream from you into the same unenviable position you're in, however I think this is still reasonable.

There is a third much less maintainable option where the CMake build tries both find_package() as well as pkgconfig to find dependencies and possibly changes the output integration files based on how it retrieved them. I really wouldn't recommend this approach if at all possible; it sounds like a nightmare of unreproducible, accidentally broken configurations.

manxorist commented 3 years ago

First, I've opened #18933 which I believe addresses all the comments you made on the initial PR, along with a bunch of other "goodness" changes. Once you're happy with the CMakeLists.txt there, I'll open a PR into the upstream libopenmpt repo (CMakeLists.txt into the root?).

Thanks for working on that!

I guess non-toplevel CMakeLists.txt will probably just get confusing for pathnames, so I think directly in the root should be fine.

Please note that we are not using git as our primary VCS. See https://github.com/OpenMPT/openmpt/blob/master/doc/contributing.md#contributing-via-github for the implications on contributing.

  1. Assume CMake users will have CMake configs available. For users that want to build using specifically the apt libraries, they can use autoconf which is entirely consistent from that environment. From your CMake build, you would publish a CMake config and optionally pc files (this is the current implementation I'm proposing). This is the most cross-platform option in my opinion.

With (1.), we would need to explicitly document CMake as not the recommended build system, because if it was, a CMake-using consuming project would start to rely on that, which would break for them if some distribution sticks to Autotools for building libopenmpt (which I frankly expect Linux distributions would do). I guess (1.) could be fine for libopenmpt only if I document it as not-recommended in the general case, and in particular that using CMake find_package would not be able to pick up libopenmpt from system libraries on most Linux systems. (1.) Also has the advantage of basically assuming the same situation for the libraries libopenmpt depends on. I.e. they could be found by find_package exclusively without implementing a pkg-config fallback, which keeps this aspect simpler as well.

  1. Only deal in .pc files. From the CMake build, use the pkg-config helpers to consume from .pc files and only publish .pc files. This is a bit unfortunate because it forces anyone downstream from you into the same unenviable position you're in, however I think this is still reasonable.

(2.) would stick to the somewhat consistent way of using pkg-config on any non-Windows platform, however from what I understand, this might be problematic for Windows projects using CMake because pkg-config cannot really express macro definitions only required for shared libraries (like LIBOPENMPT_USE_DLL), right?

  1. There is a third much less maintainable option where the CMake build tries both find_package() as well as pkgconfig to find dependencies and possibly changes the output integration files based on how it retrieved them. I really wouldn't recommend this approach if at all possible; it sounds like a nightmare of unreproducible, accidentally broken configurations.

(3.) I agree, this feels unmaintainable and very error-prone.

This problem is probably the most severe point why I feel so reluctant to use CMake, and why I feel CMake lacks proper abstraction and integration with established dependency finding technologies. According to the recent C++Now 2021 Keynote, it appears like CMake is aware of problems in this area (https://youtu.be/wULu83jQmIQ?t=3503 up to 58:50 (30 seconds total)). I really hope this results in something truly cross-platform. But it is of course future work and not of immediate use for libopenmpt or vcpkg right now.

Thomas1664 commented 2 years ago

@JackBoosY Given that #18933 updated the port libopenmpt, I think this issue can be closed now.