microsoft / vcpkg

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

[mlpack] update to 4.0.0 #27616

Closed rcurtin closed 1 year ago

rcurtin commented 1 year ago

Library name: mlpack

New version number: 4.0.0

Other information that may be useful (release notes, etc...):

Happy to help out if there are any problems! I saw @PhoebeHui did the last package update.

Blakjak88 commented 1 year ago

Any progress on this?

MonicaLiu0311 commented 1 year ago

@rcurtin I had a problem with testing usage, when building it pointed to a path that I didn't find in source, see: 01

rcurtin commented 1 year ago

@MonicaLiu0311 ah, there is now the new dependency of cereal: https://github.com/USCILab/cereal/. You will need to either let CMake find it, or you can specify CEREAL_INCLUDE_DIR in the CMake configuration. :+1:

MonicaLiu0311 commented 1 year ago

@rcurtin Because vcpkg has the port of cereal, I added cereal to the dependencies of vcpkg.json to solve this problem. Here's another problem with the build: 02

Details: ErrorList.txt Output.txt

rcurtin commented 1 year ago

Yes, I saw this one in the various MSVC builds I have done... try setting the C++ standard to C++17. You can do this by either patching CMakeLists.txt line 110 to be set(CMAKE_CXX_STANDARD 17), or I think you can specify /std:c++17 in CMAKE_CXX_FLAGS or something like that. There will be an upstream patch to handle this for version 4.0.1.

MonicaLiu0311 commented 1 year ago

Since it's a header-only library, just do this when testing: Copy your mlpack/src/ folder as a whole to the vcpkg/installed/x86-windows/include/ path, then include the path of the header file in the test case and build.

Yes, I saw this one in the various MSVC builds I have done... try setting the C++ standard to C++17. You can do this by either patching CMakeLists.txt line 110 to be set(CMAKE_CXX_STANDARD 17), or I think you can specify /std:c++17 in CMAKE_CXX_FLAGS or something like that.

I also tried your suggestion, but still the same.

There will be an upstream patch to handle this for version 4.0.1.

Is it convenient to tell the upstream patch release time? We can consider updating directly to 4.0.1.

rcurtin commented 1 year ago

Can you tell me more about how you built mlpack? I went looking, and realized that our CMake configuration should automatically set the C++ standard to C++17, so I am not sure what is happening in your case.

The release date for mlpack 4.0.1 isn't clear yet, and honestly I would prefer to figure out how to fix this case first, so that any patches that are needed here can be incorporated into that release. :)

MonicaLiu0311 commented 1 year ago

Tried multiple times still the same error. (I didn't study your port, just wanted to make sure it's available when including header files.)

For modification of portfile.ckame and vcpkg.json, see: https://github.com/microsoft/vcpkg/pull/27707/files.

The cmake project code is as follows: settting cmakelists cpp error

ErrorList.txt Output.txt

rcurtin commented 1 year ago

@MonicaLiu0311 thanks for the output! Can you try adding this patch to mlpack? https://gist.github.com/ed62ba9ea60b780b3bac5773f08ca852

Then you should get an error that indicates that mlpack programs need to be compiled with /Zc:__cplusplus and /std:c++17, and if you set those options, then the test program should compile fine.

(Technically the patch is only to give a better error message. You should be able to specify /Zc:__cplusplus and /std:c++17 for your test program in the compilation options and that should work too.)

MonicaLiu0311 commented 1 year ago

@rcurtin Thanks for your reply. The build was successful by your method (but I think this method is temporary).

If you agree, I will follow this PR to update the new version - 4.0.0, do you agree? (But other users in the same situation as me also need to solve the problem through this method)

I think you may need to make an official explanation for version 4.0.0, what do you think?

rcurtin commented 1 year ago

Awesome, glad it worked! The PR looks good to me.

Where would the right place for the official explanation be? I'll be incorporating the patch I gave you into mlpack upstream, and version 4.0.1 will come with a release note that for MSVC we must use the compiler flags above.

MonicaLiu0311 commented 1 year ago

I'll be incorporating the patch I gave you into mlpack upstream,

Ok, I will submit this PR after you incorporate this patch.

and version 4.0.1 will come with a release note that for MSVC we must use the compiler flags above.

Yep, that's what I want the official description to be seen by someone learning your library. (just a small suggestion)

rcurtin commented 1 year ago

Sounds good---the patch is open in mlpack/mlpack#3318. I updated the documentation and HISTORY.md too; please feel free to leave any comments if you like. :+1:

MonicaLiu0311 commented 1 year ago

Sounds good---the patch is open in mlpack/mlpack#3318. I updated the documentation and HISTORY.md too; please feel free to leave any comments if you like. 👍

In view of the relatively long time for this PR, I will add the patch to vcpkg first, and then remove it when you update the next version (by then you will probably have merged the patch).

rcurtin commented 1 year ago

Yep, that sounds great! :+1: