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

[symengine] Fix find TCMALLOC #33582

Closed
wants to merge 12 commits into from

Conversation

FrankXie05
Copy link
Contributor

Fix #33576

All features has been tested successfully locally.

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@FrankXie05 FrankXie05 added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Sep 6, 2023
@FrankXie05 FrankXie05 marked this pull request as ready for review September 6, 2023 09:50
@BillyONeal
Copy link
Member

BillyONeal commented Sep 7, 2023

Is the tcmalloc port not publishing something it should be publishing? I'm a bit nervous hard coding this path...

@FrankXie05
Copy link
Contributor Author

@BillyONeal The best way is probably to add port tcmalloc. I will try this。

@BillyONeal
Copy link
Member

@BillyONeal The best way is probably to add port tcmalloc. I will try this。

I think there at least needs to be an investigation; this change might still be the correct one but we should ensure tcmalloc can't publish bits that work first.

@FrankXie05
Copy link
Contributor Author

@BillyONeal The cmake integration for tcmalloc has been in progress, but since bazel and cmake, the syntax and ideas of the two build systems are very different. This progress should not be fast. And it seems they haven't updated or maintained it for a long time.
google/tcmalloc#104

@MonicaLiu0311
Copy link
Contributor

Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review".

@MonicaLiu0311 MonicaLiu0311 marked this pull request as draft September 19, 2023 02:57
@FrankXie05
Copy link
Contributor Author

@BillyONeal

@FrankXie05 FrankXie05 marked this pull request as ready for review September 19, 2023 09:36
@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Nov 13, 2023
@MonicaLiu0311
Copy link
Contributor

Flag info:reviewed for further review.

MonicaLiu0311
MonicaLiu0311 previously approved these changes Nov 13, 2023
@data-queue
Copy link
Contributor

I agree, I think the best way is to add the port tcmalloc.

@data-queue data-queue added the depends:upstream-changes Waiting on a change to the upstream project label Nov 15, 2023
@FrankXie05
Copy link
Contributor Author

@BillyONeal The best way is probably to add port tcmalloc. I will try this。

Convert to draft.

@FrankXie05 FrankXie05 marked this pull request as draft November 15, 2023 07:04
@MonicaLiu0311 MonicaLiu0311 removed the info:reviewed Pull Request changes follow basic guidelines label Nov 15, 2023
@BillyONeal
Copy link
Member

I agree, I think the best way is to add the port tcmalloc.

The port exists, it just isn't publishing a cmake config this downstream port expects.

@FrankXie05
Copy link
Contributor Author

Should we export it?

install(TARGETS libtcmalloc_minimal

@data-queue
Copy link
Contributor

I think the correct solution is to export libtcmalloc_minimal so find_package will find it.

@FrankXie05
Copy link
Contributor Author

I think the correct solution is to export libtcmalloc_minimal so find_package will find it.

Ok, I will do it.

@FrankXie05
Copy link
Contributor Author

I think the correct solution is to export libtcmalloc_minimal so find_package will find it.

Ok, I will do it.

Done.

@FrankXie05 FrankXie05 removed the depends:upstream-changes Waiting on a change to the upstream project label Mar 7, 2024
@FrankXie05 FrankXie05 marked this pull request as draft March 8, 2024 02:23
@FrankXie05
Copy link
Contributor Author

The usage has been tested successfully.

gperftools provides CMake targets:

  # this is heuristically generated, and may not be correct
  find_package(unofficial-libtcmalloc_minimal CONFIG REQUIRED)
  target_link_libraries(main PRIVATE unofficial::libtcmalloc_minimal)

@FrankXie05 FrankXie05 marked this pull request as ready for review March 8, 2024 05:51
@FrankXie05 FrankXie05 requested a review from MonicaLiu0311 March 8, 2024 05:51
MonicaLiu0311
MonicaLiu0311 previously approved these changes Mar 8, 2024
@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Mar 8, 2024
Comment on lines 199 to 204
install(
EXPORT unofficial-libtcmalloc_minimal-targets
NAMESPACE unofficial::
FILE unofficial-libtcmalloc_minimal-config.cmake
DESTINATION share/unofficial-libtcmalloc_minimal
)
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC the guidelines say that port bar may install unofficial-bar-config.cmake with unofficial::bar::foo,
and so port gperftools may install unofficial-gperftools-config.cmake with unofficial::gperftools::libtcmalloc_minimal.

If gerftools is the right port to install and own this lib.

Copy link
Member

@vicroms vicroms left a comment

Choose a reason for hiding this comment

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

Please apply @dg0yt's suggestion

@vicroms vicroms marked this pull request as draft March 8, 2024 07:49
@FrankXie05
Copy link
Contributor Author

New usage has been verified:

gperftools provides CMake targets:

  # this is heuristically generated, and may not be correct
  find_package(unofficial-gperftools CONFIG REQUIRED)
  target_link_libraries(main PRIVATE unofficial::gperftools::libtcmalloc_minimal)

@FrankXie05 FrankXie05 marked this pull request as ready for review March 8, 2024 08:47
@vicroms vicroms added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Mar 11, 2024
@vicroms
Copy link
Member

vicroms commented Mar 11, 2024

marking requires:vcpkg-team-review to confirm that we're OK with gperftools vendoring tmalloc

@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Mar 11, 2024
@kotori2
Copy link
Contributor

kotori2 commented May 5, 2024

Any update on this PR?

@BillyONeal
Copy link
Member

@JavierMatosD
@vicroms
@ras0219-msft
and I discussed today.

The totally unrelated gperftools should not be vendoring TCMALLOC. And we should not be making symengine consume that vendored copy. Sorry for any confusion.

google/tcmalloc#104 <-- Google has had more than enough time given our usual cooldown periods.

I think the path forward is this:

  • Add unofficial configs to the port tcmalloc so that CMake customers can use it. This does not necessarily imply building tcmalloc itself with CMake; we can just write configs that do the right thing.
  • Fix gperftools to stop vendoring tcmalloc and use the port
  • Fix symengine to also use the port tcmalloc

I'm sorry that our 'rotation' process has led to some confusion as we didn't always understand everything when this was handed off between maintainers. I'll look to get this one to some resolution regardless of the 'rotation' mechanism.

@BillyONeal BillyONeal self-assigned this Jun 13, 2024
@BillyONeal BillyONeal removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jun 13, 2024
@JavierMatosD
Copy link
Contributor

I'm marking this PR draft until review comments have been addressed.

@JavierMatosD JavierMatosD marked this pull request as draft October 8, 2024 21:44
@MonicaLiu0311
Copy link
Contributor

Is there any new progress?

@FrankXie05
Copy link
Contributor Author

Turn it off temporarily.

@FrankXie05 FrankXie05 closed this Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[symengine] Build failure
8 participants