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

[flecs] update to 3.2.2 #31432

Merged
merged 2 commits into from
May 17, 2023
Merged

[flecs] update to 3.2.2 #31432

merged 2 commits into from
May 17, 2023

Conversation

podsvirov
Copy link
Contributor

  • 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.

@podsvirov podsvirov changed the title Flecs update [flecs] update to 3.2.2 May 14, 2023
@MonicaLiu0311 MonicaLiu0311 added the category:port-update The issue is with a library, which is requesting update new revision label May 15, 2023
@MonicaLiu0311
Copy link
Contributor

The usage test passed (header files found):

flecs provides CMake targets:

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

@dg0yt
Copy link
Contributor

dg0yt commented May 15, 2023

target_link_libraries(main PRIVATE flecs::flecs flecs::flecs_static)

Can't there be something better than the heuristics?
(Won't my project fail in dynamic linkage when I link to flecs::flecs_static?)

@MonicaLiu0311
Copy link
Contributor

target_link_libraries(main PRIVATE flecs::flecs flecs::flecs_static)

Can't there be something better than the heuristics? (Won't my project fail in dynamic linkage when I link to flecs::flecs_static?)

  • x64-windows
  • VS 2022

When I test Usage in the following way, it succeeds. Please let me know if my tests are flawed.

usage:

flecs provides CMake targets:

    # this is heuristically generated, and may not be correct
    find_package(flecs CONFIG REQUIRED)
    target_link_libraries(main PRIVATE flecs::flecs flecs::flecs_static)
CMakeFindUsage.cpp
#include <iostream>
#include "flecs.hpp"

using namespace std;

int main()
{
cout << "Hello CMake." << endl;
return 0;
}

CMakeLists.txt
cmake_minimum_required (VERSION 3.8)

set(BUILD_SHARED_LIBS "build dynamic lib" ON)
set(CMAKE_TOOLCHAIN_FILE "E:/flecs/scripts/buildsystems/vcpkg.cmake")

project ("CMakeFindUsage")

add_library(main "CMakeFindUsage.cpp")

find_package(flecs CONFIG REQUIRED)
target_link_libraries(main PRIVATE flecs::flecs flecs::flecs_static)

@FrankXie05
Copy link
Contributor

We should use method instead of output Hello World. :)
Upstream examples: https://github.com/SanderMertens/flecs#show-me-the-code

@dg0yt
Copy link
Contributor

dg0yt commented May 15, 2023

The test is flawed because it doesn' t answer why it succeeds for you and why success is bad here.

Why does flecs::flecs_static work in dynamic linkage?
It works because the wrapper creates the missing target.
It may be nice to do this, so users/maintainers don't need to care if they come with a project which wasn't tested with the other linkage.

Why is the successful test bad here?
Suggesting target_link_libraries(main PRIVATE flecs::flecs flecs::flecs_static) only works in vcpkg. Outside vcpkg either one target will be missing, or the user will link two libs providing the same symbols but with different linkage.

What needs to be done?
Add a usage file which can work without the wrapper, i.e. using

    target_link_libraries(main PRIVATE $<IF:$<TARGET_EXISTS:flecs::flecs>,flecs::flecs,flecs::flecs_static>)

@MonicaLiu0311
Copy link
Contributor

Thanks very much for @dg0yt's patient answer.

@podsvirov
Note: I will convert this PR to draft status. Please revert to "ready for review" when you reply. Since you can't edit tags, this will let me know you've replied.

@MonicaLiu0311 MonicaLiu0311 marked this pull request as draft May 16, 2023 07:13
@podsvirov
Copy link
Contributor Author

I think this discussion goes beyond the question of updating this package.
At least these questions didn't arise in #30090, #30495 and #30929.
I suggest just merge this update.

@podsvirov podsvirov marked this pull request as ready for review May 16, 2023 21:42
@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label May 17, 2023
@JavierMatosD JavierMatosD merged commit ed3afc9 into microsoft:master May 17, 2023
@podsvirov podsvirov deleted the flecs-update branch May 17, 2023 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants