Yup really need to add a contributing.md

To run tests, see https://www.flecs.dev/flecs/md_docs_2Quickstart.html#running-tests-bake

To add a test, add a test case to this test suite: https://github.com/SanderMertens/flecs/blob/master/test/cpp/project.json#L22

Then when you run the test, a function for the test case will be generated in this file: https://github.com/SanderMertens/flecs/blob/master/test/cpp/src/Entity.cpp

Thanks for the pointers.

I wasn't able to run tests using bake:

..\bake\bake.exe run test/core -- -j 16
[  build] application core => '.'
[  error] missing dependencies! (rebuild with --trace for more details)
[  error] build interrupted for core in .
[  error] project core built with errors
[  error] failed to build '.'
[  error] build failed, cannot run

However, I was able to run them using CMake after some patches to bake.

Looking at <bake repo>/util/src/strbuf.c:280, some functions like ut_strbuf_appendstrn create an uninitialized va_list and then pass it (by copy) to another function.
This is illegal, since it accesses (by virtue of copying) uninitialized memory, causing an SEH exception on Windows. It can be fixed by initializing it to zero (something like va_list args = {0};), but I believe this is technically also illegal since va_start is the only valid way to initialize a va_list (ref) (though it should work in all scenarios I can think of).
Would you be open to another PR to bake to fix that? Otherwise, I can proceed to apply the fix only locally, which allows me to run the tests.

Then, all the non-static variants of tests exit with code 0xC0000135, which is STATUS_DLL_NOT_FOUND (source), so I'm assuming some setup is still incorrect.
But whatever, the _static variants pass, so I can work with that.

As an aside, there's just so many warnings during compilation (hundreds if not thousands of C4267, C4244, etc., mostly related to lossy implicit scalar conversions)... is that expected? :D

Now here's where I'm finally stuck: No matter what I try, I can't seem to get new test case stubs to be generated in Entity.cpp after editing project.json. Even a full build dir delete/reconfigure doesn't help.

I wasn't able to run tests using bake:

The errors you're getting from bake suggest that it can't find Flecs. After you run bake once in the repo root the test command should work and it will generate the test stub. When you run bake list it should show flecs.

No matter what I try, I can't seem to get new test case stubs to be generated

Tests can only be ran with cmake. It doesn't generate the stubs for you, that only works with bake.

Looking at /util/src/strbuf.c:280, some functions like ut_strbuf_appendstrn create an uninitialized va_list and then pass it (by copy) to another function. This is illegal

Yeah that's a known issue. The flecs repository has the same API where those issues have been fixed, but they haven't been backported to bake yet. I'd definitely be open to a PR! You could take a look at the code here https://github.com/SanderMertens/flecs/blob/master/src/datastructures/strbuf.c which also has better performance.

Otherwise, I can proceed to apply the fix only locally, which allows me to run the tests.

Odd that you run into a segfault though, since I have been running bake for years on Windows in CI 🤔 Maybe it just happens when generating new test stubs?

As an aside, there's just so many warnings during compilation (hundreds if not thousands of C4267, C4244, etc., mostly related to lossy implicit scalar conversions)... is that expected? :D

You're probably compilling the code with more strict compiler settings than the ones I used when developing it. I definitely haven't been as strict with these in bake as I have been in Flecs, so not surprised that these show up. The entire bake repository is due for a refresh, but I've been a bit preoccupied 😅

The overloads of set and assign that take const T & look superfluous. They're also using FLECS_MOV which results in a const T &&. Also, it might be nice if set did a copy or move construction instead of a default construction followed by an assignment. That's probably a bigger change though.

@indianakernick

The overloads of set and assign that take const T & look superfluous.

They're needed to support explicitly specifying T and passing a reference-to-const, like

Foo const foo{};
entity.set<Foo>(foo);

They're also using FLECS_MOV which results in a const T &&.

Yeah that does look superfluous, I'll remove that as well.

Also, it might be nice if set did a copy or move construction instead of a default construction followed by an assignment. That's probably a bigger change though.

Agreed! @SanderMertens wdyt, since this might be breaking if users rely on this for some reason?

@SanderMertens

Thanks again for all the answers!

I think I was missing the step to run bake with no arguments in the repo first. That works (at least when run from a VS command prompt).
After that, running tests fails for a different reason, pasted at the bottom. But that's fine, the new test case stubs are still generated, and I can then run them through CMake. Not great, but workable for now haha.

I'll take a look at the strbuf.c stuff as a followup sometime :)

Odd that you run into a segfault though, since I have been running bake for years on Windows in CI 🤔 Maybe it just happens when generating new test stubs?

Nope, I ran into that even before modifying project.json. Maybe the CMake defaults are stricter for some reason? Here it is, in all its glory:

You're probably compilling the code with more strict compiler settings than the ones I used when developing it. I definitely haven't been as strict with these in bake as I have been in Flecs, so not surprised that these show up. The entire bake repository is due for a refresh, but I've been a bit preoccupied 😅

I can imagine! 😉
The warnings are in flecs though, not in bake. Here's a sample:

D:\Stash\Forks\flecs\include\flecs\addons\cpp\iter.hpp(323): warning C4267: 'argument': conversion from 'size_t' to 'int32_t', possible loss of data
D:\Stash\Forks\flecs\include\flecs\addons\cpp\iter.hpp(323): note: the template instantiation context (the oldest one first) is
D:\Stash\Forks\flecs\test\cpp\src\Pairs.cpp(1288): note: see reference to function template instantiation 'A &flecs::iter::field_at<Position,Position,0x0>(int8_t,size_t) const' being compiled
        with
        [
            A=Position
        ]

But the vast majority of warnings seem to come from tests using test_int with float arguments. Maybe the test_int macro could be redefined to cast its arguments to int explicitly?

Anyways, I'll work on those tests now :D Thank you again for all your pointers.

(Error when running tests through bake:)

D:\Stash\Forks\flecs>..\bake\bake.exe run test/cpp -- -j 16
[  build] package flecs => 'D:\Stash\Forks\flecs'
[  build] application cpp => '.'
[     3%] ComponentLifecycle.cpp
ComponentLifecycle.cpp
C:\Users\juko\bake\include\flecs\addons\cpp\utils\enum.hpp(426): error C2148: total size of array must not exceed 0x7fffffff bytes
C:\Users\juko\bake\include\flecs\addons\cpp\utils\enum.hpp(426): note: the template instantiation context (the oldest one first) is
.\src\ComponentLifecycle.cpp(2748): note: see reference to class template instantiation 'flecs::component<ComponentLifecycle_compare_int8_Enum::Enum8>' being compiled
C:\Users\juko\bake\include\flecs\addons\cpp\component.hpp(392): note: while compiling class template member function 'flecs::component<ComponentLifecycle_compare_int8_Enum::Enum8>::component(flecs::world_t *,const char *,bool,flecs::id_t)'
C:\Users\juko\bake\include\flecs\addons\cpp\mixins/component/impl.hpp(12): note: see the first reference to 'flecs::component<ComponentLifecycle_compare_int8_Enum::Enum8>::component' in 'flecs::world::component'
C:\Users\juko\bake\include\flecs\addons\cpp\component.hpp(399): note: see reference to class template instantiation 'flecs::_::type<T,int>' being compiled
        with
        [
            T=ComponentLifecycle_compare_int8_Enum::Enum8
        ]
C:\Users\juko\bake\include\flecs\addons\cpp\component.hpp(272): note: see reference to class template instantiation 'flecs::_::type_impl<TestSignedEnum<int8_t>::Type>' being compiled
C:\Users\juko\bake\include\flecs\addons\cpp\component.hpp(150): note: while compiling class template member function 'flecs::entity_t flecs::_::type_impl<TestSignedEnum<int8_t>::Type>::register_id(flecs::world_t *,const char *,bool,bool,bool,flecs::id_t)'
C:\Users\juko\bake\include\flecs\addons\cpp\component.hpp(399): note: see the first reference to 'flecs::_::type_impl<TestSignedEnum<int8_t>::Type>::register_id' in 'flecs::component<ComponentLifecycle_compare_int8_Enum::Enum8>::component'
C:\Users\juko\bake\include\flecs\addons\cpp\mixins/component/impl.hpp(12): note: see the first reference to 'flecs::component<ComponentLifecycle_compare_int8_Enum::Enum8>::component' in 'flecs::world::component'
C:\Users\juko\bake\include\flecs\addons\cpp\component.hpp(187): note: see reference to function template instantiation 'void flecs::_::init_enum<T,0>(flecs::world_t *,flecs::entity_t)' being compiled
        with
        [
            T=TestSignedEnum<int8_t>::Type
        ]
C:\Users\juko\bake\include\flecs\addons\cpp\utils\enum.hpp(431): note: see reference to class template instantiation 'flecs::_::enum_type<E>' being compiled
        with
        [
            E=TestSignedEnum<int8_t>::Type
        ]
[  error] command returned 2
   cl.exe /nologo /EHsc /Od -D__BAKE__ -D__BAKE__ /DBAKE_PROJECT_ID=\"cpp\" /Dcpp_EXPORTS /I C:\Users\juko\bake\include /I .\include /c .\src\ComponentLifecycle.cpp /Fo.bake_cache\x64-Windows-debug\obj\ComponentLifecycle.obj /Zi
[   from] command for task 'ComponentLifecycle.cpp' failed
[   from] dependency 'objects' failed
[   from] failed to build rule 'ARTEFACT'
[  error] build interrupted for cpp in .
[  error] project cpp built with errors
[  error] failed to build '.'
[  error] build failed, cannot run

The warnings are in flecs though, not in bake. Here's a sample:

Ah I thought you meant the var_args thing. The unit tests are not compiled with strict warning settings, that's intentional. A lot of tests rely on the implicit conversion to avoid comparing two floats.

Btw the reason CI is failing is because the flecs.c/flecs.h files in distr don't match the source. If you run bake once in the repository it should sync them.

Agreed! @SanderMertens wdyt, since this might be breaking if users rely on this for some reason?

The challenge with calling a move/copy ctor for set is that the C++ code doesn't know whether the component just got constructed. I could add a code path that bubbles up this information, but that'd add overhead to internals and branching (if created do move_ctor, else do move).

It could make sense to support an operation that always inserts a new component and asserts when it already exists.

cc @indianakernick

Got it. distr/* should be good now.

The challenge with calling a move/copy ctor for set is that the C++ code doesn't know whether the component just got constructed. I could add a code path that bubbles up this information, but that'd add overhead to internals and branching (if created do move_ctor, else do move).

I don't know enough about flecs internals to contribute to that discussion so I'll stay on the sidelines here 😉

LGTM! Apologies for the latemerge