Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[or-tools] adds or-tools as new port #42480

Draft
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

JulZimmermann
Copy link
Contributor

@JulZimmermann JulZimmermann commented Dec 2, 2024

Adds Google or-tools lib as a port.


  • Changes comply with the maintainer guide.
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See adding-usage for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

@JulZimmermann JulZimmermann changed the title Or tools [or-tools] adds or-tools as new port Dec 2, 2024
@Mengna-Li Mengna-Li added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Dec 3, 2024
@BillyONeal
Copy link
Member

Are there library components here or is this entirely providing a tool? We generally don't put tooling in our registry unless there is some kind of interdependency with the exact tool and some other library component, because we believe binary distribution systems like nuget or apt or dnf are a better fit for those situations. For example, protoc must match the libprotobuf you get exactly for the resulting generated bits to be link compatible, so we will distribute that.

I'm marking this PR as a draft because several builds are failing, please either fix the supports expression to describe what the port actually supports and mark 'ready for review' afterwards.

@BillyONeal BillyONeal marked this pull request as draft December 4, 2024 04:40
@JulZimmermann
Copy link
Contributor Author

This is a library for solving optimization problems. The executables build also (the tools) are not really needed. Your right. I'll try to disable the build of them.

I'll also have a look at the supported platforms

@JulZimmermann JulZimmermann marked this pull request as ready for review December 5, 2024 05:08
ports/or-tools/portfile.cmake Outdated Show resolved Hide resolved
ports/or-tools/portfile.cmake Outdated Show resolved Hide resolved
ports/or-tools/portfile.cmake Outdated Show resolved Hide resolved
ports/or-tools/portfile.cmake Outdated Show resolved Hide resolved
ports/or-tools/portfile.cmake Outdated Show resolved Hide resolved
ports/or-tools/vcpkg.json Outdated Show resolved Hide resolved
ports/or-tools/vcpkg.json Outdated Show resolved Hide resolved
@Mengna-Li Mengna-Li marked this pull request as draft December 6, 2024 01:31
@JulZimmermann JulZimmermann marked this pull request as ready for review December 10, 2024 06:46
@Mengna-Li
Copy link
Contributor

@JulZimmermann I encountered the following error while testing the usage:

CMake Error at CMakeProject2/CMakeLists.txt:14 (target_link_libraries):
1> [CMake]   Error evaluating generator expression:
1> [CMake] 
1> [CMake]     $<TARGET_OBJECTS:ortools_math_opt_core>
1> [CMake] 
1> [CMake]   Objects of target "ortools_math_opt_core" referenced but no such target
1> [CMake]   exists.

@JulZimmermann
Copy link
Contributor Author

@Mengna-Li I disabled the build of the math_opt targets. Now only the really needed ortools target is build.

@JulZimmermann
Copy link
Contributor Author

Unfortunately, there still seems to be a problem.
To use BUILD_MATH_OPT=OFF I also had to set USE_GUROBI=OFFwhich makes sense since gurobi is a proprietary third-party solver.

However, with these settings I have linker errors on linux now. Looks like a bug in the cmake. I'll need to investigate this further.

@JulZimmermann JulZimmermann marked this pull request as draft December 13, 2024 07:52
@dg0yt
Copy link
Contributor

dg0yt commented Dec 13, 2024

On b135bb7: Is this expected to pick system libraries if available?
(Reusing the cached artifact will not install system libraries...)

@JulZimmermann
Copy link
Contributor Author

@Mengna-Li It seems USE_GUROBI=OFF isn't supported (See: https://github.com/google/or-tools/blob/8edc858e5cbe8902801d846899dc0de9be748b2c/CMakeLists.txt#L256C1-L256C73)
Therefore BUILD_MATH_OPT=OFF is also not supported and need to be build.

However, I added a using file describing that only the ortools::ortools CMake Target should be used

@JulZimmermann
Copy link
Contributor Author

@dg0yt No, system libs shouldn't be used. Did I make a mistake in the port file so they are picked?

@JulZimmermann JulZimmermann marked this pull request as ready for review December 17, 2024 12:59
@dg0yt
Copy link
Contributor

dg0yt commented Dec 17, 2024

It seems USE_GUROBI=OFF isn't supported

Why shouldn't it be supported to turn it off? That CMake line doesn't enforce it, but adds preqrequisites.
This is one of the "system libs":

@JulZimmermann
Copy link
Contributor Author

@dg0yt I'm not sure why the flag exists if it can't be used. That was confusing me too.
If you compile the lib with USE_GUROBI=OFF you get linker errors when trying to use the lib.

Fromor-tools CMakeListst.txt:

## GUROBI
# Since it is dynamicaly loaded upon use, OFF is currently not supported.
CMAKE_DEPENDENT_OPTION(USE_GUROBI "Use the Gurobi solver" ON "BUILD_CXX" OFF)
message(STATUS "Gurobi support: ${USE_GUROBI}")

In my opinion these options shouldn't exist if they can't be used.

However, because the gurobi lib would be "dynamicaly loaded upon use" there is no hard dependency on gurobi even when USE_GUROBI=OFF is not set

@dg0yt
Copy link
Contributor

dg0yt commented Dec 18, 2024

If you compile the lib with USE_GUROBI=OFF you get linker errors when trying to use the lib.

Upstream bug IMO.

However, because the gurobi lib would be "dynamicaly loaded upon use" there is no hard dependency on gurobi even when USE_GUROBI=OFF is not set

I see...


vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
FEATURES
highs USE_HIGHS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that your format has been restored to its original state.

@Mengna-Li
Copy link
Contributor

Note: I will be converting your PR to draft status. When you're ready, please revert to "ready for review". That way, I can be aware that you've responded since you can't modify the tags.

@Mengna-Li Mengna-Li marked this pull request as draft December 23, 2024 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants