build2-packaging / imgui

Build2 package for imgui
Other
2 stars 5 forks source link

Adding example_null backend as test for libimgui #7

Closed Swat-SomeBug closed 1 year ago

Swat-SomeBug commented 1 year ago

Upstream uses example_null to test in headless environments.

The PR adds the code in example_null as a new package to test libimgui.

Klaim commented 1 year ago

I didn't try it yet, but it looks fine.

However, as a potential improvement: ideally I would like the example_null to also appear in the examples, so that the example package looks like the original project (when people look for the things). I'm not sure how to achieve that exactly without duplications of project though.

Swat-SomeBug commented 1 year ago

However, as a potential improvement: ideally I would like the example_null to also appear in the examples, so that the example package looks like the original project (when people look for the things). I'm not sure how to achieve that exactly without duplications of project though.

To achieve this: imgui-examples can itself be marked as tests for libimgui with null as the backend. null can always be built and other examples are conditionally built. Only null can be marked for test. So basically move the package into imgui-examples a directory.

Swat-SomeBug commented 1 year ago

However, as a potential improvement: ideally I would like the example_null to also appear in the examples, so that the example package looks like the original project (when people look for the things). I'm not sure how to achieve that exactly without duplications of project though.

To achieve this: imgui-examples can itself be marked as tests for libimgui with null as the backend. null can always be built and other examples are conditionally built. Only null can be marked for test. So basically move the package into imgui-examples a directory.

Another line of thought is build2 distinguishes between tests and example, which from a structure point of view, Imgui doesn’t. It’s good to have this distinction, which is more idiomatic to build2, because libimgui itself could be built and tested without needing to build (not execute) all possible backends based on $cxx.target.system that in turn have system dependencies on OpenGL and Vulkan libraries. But may cause surprises to consumers if they expect exact directory structure as upstream.

At the moment, I believe having a separate package explicitly for testing is a good idea but to make it more explicit to users, maybe libimgui-test could be renamed to libimgui-null-backend-test?

Klaim commented 1 year ago

At the moment, I believe having a separate package explicitly for testing is a good idea but to make it more explicit to users, maybe libimgui-test could be renamed to libimgui-null-backend-test?

This seems a reasonable compromise to me yes. Thanks for exposing all the options. :)

Rookfighter commented 1 year ago

Personally, I would prefer to have separate packages for examples and tests and not mix them using any conditional build options. IMO that would just complicate consuming the package.

In the context of Dear ImGui, I think it is important to ask: Is the null backend example typically used as "test" by the developers? Then I would definitely see it in a test package and not a example package in the world of build2.

To follow this, I don't really see how the null backend example is a "example" for users of the library. It seems more like a sanity check or smoke test for the library itself. There's also a comment in the file which says This is useful to test building, but you cannot interact with anything here!.

So all in all, I think having the null example in a test package and not in the examples package is totally fine.

Klaim commented 1 year ago

So all in all, I think having the null example in a test package and not in the examples package is totally fine.

I agree with all your points, but the name of that directory/project in imgui and it's location and also how it is presented in the original repo suggests that it's an "example" for a reason. I would prefer it to be moved in tests directories upstream, but as it's not the case and it's preferable not to change the meaning conveyed from upstream (even if we disagree in some ways). Which is why I pointed this initially.

But the current compromise does put this example as a separate test package while not stating it's just tests (because it's not really just that), so I guess we are both fine with it?

Rookfighter commented 1 year ago

So all in all, I think having the null example in a test package and not in the examples package is totally fine.

I agree with all your points, but the name of that directory/project in imgui and it's location and also how it is presented in the original repo suggests that it's an "example" for a reason. I would prefer it to be moved in tests directories upstream, but as it's not the case and it's preferable not to change the meaning conveyed from upstream (even if we disagree in some ways). Which is why I pointed this initially.

But the current compromise does put this example as a separate test package while not stating it's just tests (because it's not really just that), so I guess we are both fine with it?

Yeah, I'm totally fine with the current compromise. It was not my intention to argue for a pure libimgui-test package. The way it is now is good. The test is in a separate package and its name is expressive enough to understand that it just uses the null backend example for "testing".