search

build2-packaging / imgui

Build2 package for imgui
Other
2 stars 5 forks source link

Split imgui backends into separate packages #2

Closed Rookfighter closed 2 years ago

Rookfighter commented 2 years ago

Hi everybody,

I finally managed to finish a first draft for splitting the backends of imgui. Basically I created one build2 package per backend. Each package is postfixed by its backend-type (either render or platform) and the implementation which it is based on, e.g. metal or opengl.

A few general notes:

Feedback and suggestions are welcome

Disclaimer: As of this writing the latest CI build on GitHub is still in progress. If it is broken I will fix it ASAP, but I opened the PR already to give you a chance to comment on what I have done so far.

Klaim commented 2 years ago

I cloned and built successfully the example package locally on Windows 11 + msvc (2022 preview), I tried all the current examples (knowing that some are missing) and they all work as expected. Good job!

I'll also try on linux tomorrow.

Klaim commented 2 years ago

I also tried to create a new project and depend on this repo's packages, but it fails at initialization:

$ bdep init
initializing in project E:\projects\build2-packaging\dearimgui\usertest\myapp\
synchronizing:
  update glfw/3.3.4+1 (required by imgui-platform-glfw)
  new imgui-core/1.86.0-a.0.20220601215117 (required by imgui-platform-glfw, imgui-render-opengl3)
  new imgui-platform-glfw/1.86.0-a.0.20220601215117 (required by myapp)
  new opengl-meta/1.0.0-a.0.20220601235057.b9271584abf6 (required by imgui-render-opengl3)
  new imgui-render-opengl3/1.86.0-a.0.20220601215117 (required by myapp)
  new myapp/0.1.0-a.0.19700101000000
checking out imgui-core/1.86.0-a.0.20220601215117
distributing imgui-core/1.86.0-a.0.20220601215117
error: distribution of uncommitted project ..\build-msvc\.bpkg\tmp\63c3ca6500b9\imgui-core\
  info: specify config.dist.uncommitted=true to force
 84% of targets distributed

This is reproducible by just b dist ... from one of the packages sources after cloning.

This will probably be fixed once you remove -a.0.z from version names in manifest.

Using bdep init --build-option config.dist.uncommitted=true instead works around the issue.

Then it leads to linknig errors (when using the code copy pasted from example_glfw_opengl3 into the main source of a bdep new -t exe project. The link errors are because some symbols are imported/exported while I used the static targets. This shows that something is wrong with the symbol import/export dance. Note that most imgui users will use the static versions only (and the doc recommend to not use the shared library version, see imguiconfig.h comments for example) so this scenario needs to work as a priority. Once I switched to shared libraries for imgui, the issue was fixed and the program works as expected 👍🏽

Here is the app in case you want to test (it's quick to reproduce but it's handy to keep it around in a repo): https://github.com/Klaim/build2-test-usage-imgui

boris-kolpackov commented 2 years ago

Nice work!

I am have no clue how to build executables from objective-C code, which also depend on build2 packages, so I would need some help to get the macos examples running.

Here is an example that will hopefully get you started:

libs =
import libs += libhello%lib{hello}

libue{test-meta}: $libs

exe{test}: obje{test} libue{test-meta}
{{
  rpaths = $cxx.lib_rpaths($<[1], exe)
  libs = $cxx.lib_libs($<[1], exe)

  depdb hash rpaths
  depdb hash libs

  diag ld $>
  $cxx.path $cc.coptions $cxx.coptions $cxx.loptions $cc.loptions $cxx.mode \
     $rpaths -o $path($>) $path($<[0]) $libs $cxx.libs $cc.libs

}}

Let me know if you hit any issues or something is unclear.

boris-kolpackov commented 2 years ago

@Klaim

error: distribution of uncommitted project ..\build-msvc.bpkg\tmp\63c3ca6500b9\imgui-core\

This should definitely not happen: a just-cloned project is being distributed and it shouldn't have anything uncommitted. I tried to reproduce this with your example but for me it fails like this:

error: unable to satisfy dependency constraint (imgui-platform-glfw == 1.86.0-a.0.20220601215117) of package myapp
  info: available imgui-platform-glfw versions: 1.86.0-a.0.20220601215116.ce74cec38637

If you still can reproduce the original issue, could you cd ..\build-msvc\.bpkg\tmp\63c3ca6500b9\imgui-core\ and then run git status to see what exactly is uncommitted there?

boris-kolpackov commented 2 years ago

@Klaim

I then went ahead and changed the version constraints in build2-test-usage-imgui/manifest to:

depends: imgui-platform-glfw >= 1.86.0-
depends: imgui-render-opengl3 >= 1.86.0-

And now bdep init succeeds and I get imgui-core/1.86.0-a.0.20220601215116.ce74cec38637.

Klaim commented 2 years ago

@boris-kolpackov

I initially tried with no constraint, then ==, and now I tried the constraints that work for you, in all cases I get that error.

However, I suspect that I'm using an old staging version, so I need to upgrade it to confirm.

If you still can reproduce the original issue, could you cd ..\build-msvc.bpkg\tmp\63c3ca6500b9\imgui-core\ and then run git status to see what exactly is uncommitted there?

The ..\build-msvc\.bpkg\tmp directory is deleted when that happen so I can't look into it.

Klaim commented 2 years ago

@boris-kolpackov I still have the issue I reported with last stage build2 and another different error with v0.14.0. I'm moving discussion about this there to avoid adding noise here. I put a complete log there.

Klaim commented 2 years ago

I attempted to build this version on Windows/msvc, the vulkan package fails to build. I modified it locally to use VulkanHeaders but that then lead to some linking issues, I'm trying to find why (I'm pretty sure it was working fine before).

Klaim commented 2 years ago

https://github.com/Rookfighter/imgui/pull/1 This works for me as soon as I have installed the Vulkan SDK (I forgot because I switched to a new computer recently) and specify where to find the lib file in the SDK.

It might be worth handling the vulkan case in a separate PR though, it's a bit rough.

Klaim commented 2 years ago

I tried this last version:

I'm not sure if it's a good idea or to have the vulkan/gl apis as implementation (as in non-exported) dependencies instead of interface (as in exported) dependencies. To me it seems ok as is and we could change that later if it becomes problematic.

Other than that, the pull request looks good. The end-of-files can be quickly fixed I guess. There is a few examples missing (though they could be added in another PR).

One potential issue: @boris-kolpackov mentionned there that these packages are using v0.15.0 features while being set to v0.14.0. Apparently that leads to problems (like the one reported for v0.14.0 in the linked thread, which still exist if I try to bdep init my test app using imgui). I think at some point we will need to require build2 >= v0.15.0 instead of currently >= v0.14.0 as v0.14.0 will simply not work.

Rookfighter commented 2 years ago

@Klaim Having 0.14.0 as required build2 is just plain wrong, if 0.15.0 features are used. I will fix it, thanks for pointing me at it :+1:

About the missing exports for opengl3 and vulkan: That's weird, the vulkan and opengl dependencies are clearly declared as "exports". Otherwise the example would fail linking, too. @boris-kolpackov Do you have any thoughts on this? Is it because I omit any version for the dependency packages vulkan-meta and opengl-meta? Seems to be a problem with the meta packages...? Or am I missing something here? I cannot reproduce the linking problem with a fresh build2 project on my Linux machine. Also @Klaim are you sure the linking problem only happens with opengl3 and opengl2 is fine? Essentially the build files are identical, except for the version number!

For the missing MacOS examples, I guess I will not fix them in the scope of this PR. I do not own any Mac afterall, so maybe we could find somebody, who has the necessary hardware...? It's quite tiresome to do all the compilation cloud-based :D

Klaim commented 2 years ago

About the missing exports for opengl3 and vulkan: That's weird, the vulkan and opengl dependencies are clearly declared as "exports". Otherwise the example would fail linking, too.

I'll investigate, this is weird.

Klaim commented 2 years ago

@Rookfighter Nevermind, I retried to reproduce the issue but it doesn't appear, everything link as expected. Not sure what I did before but linking work as expected without having to specify the meta packages. Either I did something wrong before or the commit changing the build2 version changed something. In any way you can ignore it, sorry for the noise!

The code is there by the way: https://github.com/Klaim/build2-test-usage-imgui just init and build it, it's using the same code as the examples.

I only tried with opengl3 and vulkan as a quick test, didn't try the others.

Klaim commented 2 years ago

For the missing MacOS examples, I guess I will not fix them in the scope of this PR. I do not own any Mac afterall, so maybe we could find somebody, who has the necessary hardware...? It's quite tiresome to do all the compilation cloud-based :D

I hear you ^^ totally fine.

I was actually thinking about the example_null one (which could be marked as a test actually, making the example package a basic test with just the example_null running?) and the emscripten one. Actually I can probably help with the emscripten one, and both can be done in a separate PR. 👍🏽

EDIT> For info, if you want to try the emscripten one, just install emscripten somewhere, run the activation script (to make it enabled in yuor environnement) , create a configuration with config.cxx=em++ config.c=emcc and you're ready to build that example project. The rest of what's necessary is setting the emscripten-specific flags. Emscripten is basically clang + a wrapper over it to optionally add libraries (with specific versions provided by emscripten itself) and linking options. If it works for you, it will probably work on all platforms the same as it targets WASM32.

boris-kolpackov commented 2 years ago

For the missing MacOS examples, I guess I will not fix them in the scope of this PR. I do not own any Mac afterall, so maybe we could find somebody, who has the necessary hardware...?

Yes, that's perfectly reasonable.

It's quite tiresome to do all the compilation cloud-based :D

FYI, our CI service has an interactive mode where you can set a "breakpoint" in the CI process and you get a login to the VM once it reaches that point. It's currently not yet generally available but we are using it quite a bit internally. Let me know if you would like to give it a try (but, as I said, if you would rather let someone else deal with Mac, that's also perfectly fine).

Klaim commented 2 years ago

I think this is mergeable or close to be:

One thing I dont get is why github isn't picking the CI files yet, this branch is not being tested. If I remember correctly it should be doing so?

Anyway, good job! LGTM

boris-kolpackov commented 2 years ago

I would like to ask for a few more days to review this properly (have been swamped with other things for the past couple of days).

In the meantime, it would be good to submit this to ci.stage.build2.org (if using staged toolchain, bdep-ci submits there by default) and make sure it builds on all the relevant platforms. If some CI VMs are missing any system/pre-installed libraries, let me know and I will try to add them.

Also, I was looking at the set of files that Debian's libimgui-dev package contains (it's a good idea to be consistent with major distributions WRT to library/header names so that people can satisfy dependencies with distribution packages; see sys: package schema for details). Here is the Debian package list: https://packages.debian.org/sid/amd64/libimgui-dev/filelist

I am a bit fuzzy on where Debian's backend libraries are -- my guess is they put everything into a static library and it's up to the user to satisfy any dependencies for the backends that they use.

But looking at the headers, this definitely doesn't match how/where we install them (plus we appear to install private/internal headers for libimgui, which is not ideal). I think unless there is a good reason to deviate, we should follow Debian and install all headers into include/imgui/ and backend headers into include/imgui/backends/ (while most headers are prefixed with imgui and would therefore be safe to install to include/, there are few that instead are prefixed with imstb, but that's probably unique enough and we could let it slide). Looking at the upstream examples, I see they include both core and backend headers without any prefixes. This can be arranged in 0.15.0 as shown, for example, here: https://github.com/build2-packaging/Qt6/blob/master/libQt6Core/QtCore/buildfile#L1127

Rookfighter commented 2 years ago

@boris-kolpackov Alright, no worries. I still have to work on a few points from @Klaim.

Concerning the headers I agree and actually at first I had the install directories just the way the Debian package has them, but then changed it, because the examples use unprefixed headers. I guess build2 will still be able to find the headers as I export the correct include directory even if I change the install directory. Will fix that!

Concerning private headers: I would see imgui_internal.h as private candidate, but it is also part of the Debian package. So which way should we go?

In this regard there's another point: should we rename the package to libimgui? TBH (and this is totally a personal opinion) I do not like the lib prefix for library packages, but I would go that way if you think it's better. Actually, I would prefer to use the name that the library is commonly known for, e.g. imgui, sfml, sdl, but also libpng or libjpeg. Also I think the lib prefix is mostly a Unix thing and not really common in the windows world, so enforcing it seems contradictory for a cross platform tooling (please, correct me if I am wrong!).

Rookfighter commented 2 years ago

@boris-kolpackov and @Klaim I just tried building the package with different compiler options, e.g.:

cd <path/to/repo>/
b config.cc.coptions="-O3 -DNDEBUG"

If I do this in the repo root directory it does not apply the options during the build process. I actually have to change into a concrete package directory, e.g.

cd imgui-examples/
b config.cc.coptions="-O3 -DNDEBUG"

This then works and the package is cleanly rebuilt and the flags are applied during compilation. Any thoughts how I can propagate these flags from the repo root level instead of changing into each individual package directory (e.g. for Debug vs Release build in the CI)?

boris-kolpackov commented 2 years ago

I guess build2 will still be able to find the headers as I export the correct include directory even if I change the install directory.

Just to be clear, there are two part to this working properly: For the non-installed case, you are correct, you just need to export suitable poptions. But for the installed case you need to make sure that suitable -I options are added to pkg-config files. This is achieved with the new (as of 0.15.0) mechanism that I linked to an example of. In this case we will need something along these lines:

lib{imgui}: cxx.pkgconfig.include = include/imgui/ include/imgui/backends/

Concerning private headers: [...]

Ah, you are right. In fact, it seems like we are missing some headers that Debian has. Not sure if this is intentional.

In this regard there's another point: should we rename the package to libimgui.

Yeah, this is a thorny question. I have some thoughts on this here: https://github.com/build2/HOWTO/blob/master/entries/name-packages-in-project.md

The short of it is: if I were packaging it, I would call it libimgui but it's up to you whether you want to go this route or not. If you want to stick with just imgui, that's fine.

If I do this in the repo root directory it does not apply the options during the build process.

By default command line variable overrides are applied to the current project's amalgamation. But what you have in the root of your repository it a "glue buildfile" (you can search the build system manual for this term to learn more) which lives on its own and "pulls" (imports) other projects (packages of your repository). So, as a result, the override applies to the glue project but not to the project that it pulls. I think the easiest fix in this case is to use global override instead (as the name suggests, it applies to everything):

b '!config.cc.coptions=-O3 -DNDEBUG'

Long-term, however, you would probably want to switch to using bpkg for this (similar to what our CI service does) since it will be hard to handle more complex (say, with build-time dependencies) case with your manual approach (in fact, I don't even know how you manage to satisfy imgui's dependencies). I guess the overall moral of the story is: there were reasons why we spent a ton of effort on inventing our own CI infrastructure and you are beginning to discover those reasons ;-)

boris-kolpackov commented 2 years ago

Also a couple of overall comments that I couldn't do inline:

Rookfighter commented 2 years ago

@boris-kolpackov I think I addressed most of your points by now. I changed the package names to the prefixed libimgui format. I also updated the dependencies opengl-meta and vulkan-meta to use the lib prefix now. Just for the sake of consistency.

Concerning missing headers: I believe you refer to imgui_freetype.h and imgui_stdlib.h. The first one makes imgui use freetype to render fonts and the latter one allows to use std::string for text widgets (as imgui seems to be independent of stl...?). These are optional parts of imgui as they introduce dependencies. I would prefer to leave the choice of dependencies in the hands of package consumers (maybe as separate packages again?). Anyway, I do not see them as part of this PR. Maybe again another point for a new follow-up ticket.

Concerning mingw: I will try to work out why the compilation with mingw is failing. I also tried using bdep ci, but I am getting the following error:

~/develop/imgui (feature/split-backends=) $ bdep ci
submitting:
  to:      https://ci.stage.build2.org
  in:      https://github.com/Rookfighter/imgui.git#feature/split-backends@d38fa1fead9ee74543deb2f2f8c1587dd8291da7

  package: libimgui
  version: 1.86.0-a.0.20220610183728.d38fa1fead9e

  package: libimgui-platform-glfw
  version: 1.86.0-a.0.20220610183728.d38fa1fead9e

[... list of packages ...]

  package: libimgui-examples
  version: 1.86.0-a.0.20220610183728.d38fa1fead9e
continue? [y/n] y
######################################################################## 100,0%
error: unable to fetch repository information (run 'bpkg rep-info --deep --manifest https://github.com/Rookfighter/imgui.git#feature/split-backends@d38fa1fead9ee74543deb2f2f8c1587dd8291da7' for details)
  info: reference: 42fdfdd2-8528-4fb5-80dd-67286fd15a80

The suggested command just bloats me with contents of license and readme files, so it was not really a help. Any idea what the issue might be?

@Klaim Haven't forgotten about your point, I will be working on Release and Debug mode in GitHub CI. I want to take a closer look to what Boris proposed with bpkg but I didn't have the time yet to do some reading on it.

Klaim commented 2 years ago

@Klaim Haven't forgotten about your point, I will be working on Release and Debug mode in GitHub CI. I want to take a closer look to what Boris proposed with bpkg but I didn't have the time yet to do some reading on it.

No worries, thanks for your work!

BTW I'm starting to think we could make meta packages for each of these benchmark to help with some other projects - but please don't bother doing this now, we'll see later.

boris-kolpackov commented 2 years ago

I see you added the lib prefix, even to imgui-examples, which definitely should not have the lib prefix (since it's not a library).

boris-kolpackov commented 2 years ago

@Rookfighter I've also noticed your unsuccessful attempt to CI this. I was able to reproduce this and we are looking into it.

boris-kolpackov commented 2 years ago

@Rookfighter Ok, the stage CI should be fixed now, here is a test submission I've just made: https://ci.stage.build2.org/@318a29c8-f556-4983-ba56-c906ad9f04c8 Let me know if any configuration are missing any system packages (e.g., OpenGL on Linux) and I will try to add them.

Rookfighter commented 2 years ago

@boris-kolpackov Concering the lib prefix of the examples package: True, I was wondering what would be the correct variant. Since the examples package actually contains examples for the libimgui package (and not any utility applications) I thought libimgui-examples would be the correct name. Anyway, will fix it.

Rookfighter commented 2 years ago

@boris-kolpackov Thanks for fixing the bdep ci issue! Works now also on my side.

I think I will leave the mingw support for a follow-up PR, as I can't get it to work straight away and mingw is also not really my priority, right now.

@Klaim I added config files as you suggested (without the common config file for now). However the options are not applied during build time. For example when I use bdep init -C @gcc --options-file config/gcc.release.build I get the following output

$ bdep init -C @gcc --options-file config/gcc.release.build 
initializing in project /home/fabian/develop/imgui/
../imgui-gcc/build/config.build: warning: dropping no longer used variable config.cc.coptions
  info: variable value: '-O3 -DNDEBUG'

When I use b --options-file config/gcc.release.build I do not see any warnings / errors, but the config parameters still do not get applied. Any ideas?

Klaim commented 2 years ago

Any ideas?

Weird, I dont remember ever seeing this. I'll take a look ASAP.

Klaim commented 2 years ago

I managed to reproduce the warning, you used:

$ bdep init -C @gcc --options-file config/gcc.release.build

In that example you did not specify the kind of build configuration, which should be cc I guess, that should appear after the configuration name/directory and before the options. I get that warning only when I don't set this field, probably because build2 assume the next field should be something else.

$ bdep init -C @gcc cc config.cxx=g++ --options-file config/gcc.release.build

Works for me without the warning.

BTW I would have preferred the compiler name set in the config files too, so that the user only have to specify the option file (and the CI only need to use the right file). Also, for msvc debug you need the config.cxx=cl /MDd otherwise it will not work as expected. For example here is the content of one of my projects using SFML for msvc debug:

config.cxx=cl /MDd
config.cc.coptions += /DEBUG /Od /Zi
config.cc.loptions += /DEBUG

However if you have a reason to keep the compiler name given by the CI, then nevermind. In that project using SFML I setup the CI to use these files so I think it should work.

Other than that I just tried with msvc debug and release and it initializes without warnings and builds some examples as expected. Same for init with g++ on linux/ubuntu (I couldnt build because of missing X11 dependencies or something like that)

boris-kolpackov commented 2 years ago 
$ bdep init -C @gcc --options-file config/gcc.release.build

The better way to load extra configuration files is with the config.config.load variable:

$ bdep init -C @gcc cc config.config.load=config/gcc.release.build
boris-kolpackov commented 2 years ago

Quick question: Is comparing cxx.target.system to 'mingw32' the correct way to check for mingw? I took this from here, but I also see the possibility to check via cxx.id == gcc. Which one is recommended way?

The *.target.system is the correct way: you can also use Clang to build for MinGW. And in parallel universe GCC might target a different runtime on Windows (say, MSVC).

Rookfighter commented 2 years ago

@Klaim I added the compiler specification to the options files. @boris-kolpackov I am still using the --options-file flag as I got the following error with config.config.load:

$ bdep init -V -C @gcc cc config.config.load=build-config/gcc.release.build
mkdir /tmp/bdep-12013-0/
initializing in project /home/fabian/develop/imgui/
mkdir /home/fabian/develop/imgui/.bdep/
bpkg --verbose 3 create -d /home/fabian/develop/imgui-gcc --name gcc --type target --no-host-config --no-build2-config cc config.config.load=build-config/gcc.release.build
mkdir -p /home/fabian/develop/imgui-gcc/
b --verbose 3 config.config.load=build-config/gcc.release.build "create('/home/fabian/develop/imgui-gcc/', cc)"
mkdir /home/fabian/develop/imgui-gcc/build/
cat >/home/fabian/develop/imgui-gcc/build/bootstrap.build
cat >/home/fabian/develop/imgui-gcc/build/root.build
cat >/home/fabian/develop/imgui-gcc/buildfile
build-config/gcc.release.build: error: incompatible config file build-config/gcc.release.build
  info: config file version   0 (missing)
  info: config module version 1
  info: consider reconfiguring @/home/fabian/develop/imgui-gcc/
rmdir -r /tmp/bdep-12013-0/

Nevertheless, I think this PR might (finally) be nearing its end. One last question: After this PR is merged, should we rename the repo to libimgui, too? Or leave it as imgui? I guess renaming it would invalidate all existing references to this repo in outher packages / projects. But renaming would also be more consistent with other package repositories like libfreetype. Any thoughts?

boris-kolpackov commented 2 years ago

After this PR is merged, should we rename the repo to libimgui, too? Or leave it as imgui? I guess renaming it would invalidate all existing references to this repo in outher packages / projects. But renaming would also be more consistent with other package repositories like libfreetype. Any thoughts?

I think for the repository itself using upstream name is a good idea, especially if it contains (or may contain in the future) non-library packages.

I got the following error with config.config.load [...]

This mechanism expects a real configuration file (like config.build; it's often created with config.config.save) and which must include a version. Try adding the following as a first non-comment line in your gcc.release.build file:

config.version = 1
boris-kolpackov commented 2 years ago

@Rookfighter BTW, what is the status of CI (as submitted with bdep ci to ci.stage.build2.org)? Ideally, we would at least want successful builds for the main platform/compiler combinations. Maybe add a link to the latest CI submission here?

Rookfighter commented 2 years ago

@boris-kolpackov Thanks, I switched to using config.config.load. The version was what was missing.

Here' a bdep ci run with the latest commit: https://ci.stage.build2.org/@161ea517-24de-4c17-bb06-cce35a4f7657

Swat-SomeBug commented 2 years ago

Hi @Rookfighter awesome job with the PR. I noticed one point which may not be directly related to this PR, but in general when a glue buildfile is used to the selectively include packages (based on cxx.target.class as in this case), bdep and b behave differently. During bdep init, the top level glue buildfile seems to be ignored (Not sure, just guessing?) and all the packages are initialized even when they are not meant to be built. As I understand it, b would build the default config but considers the glue buildfile and builds everything correctly. bdep update on the other hand, builds all the initialized packages which then causes build errors (building libimgui-render-metal on linux for example). @boris-kolpackov is this observation inline with the current implementation? Is there a way to selectively initialize package using the glue buidlfile?

Tested on:

$ b --version
build2 0.15.0-a.0.2f29c7fbe758
libbutl 0.15.0-a.0.0ae2c768fc8b
$ bdep --version
bdep 0.15.0-a.0.ed05ed4c87df
libbpkg 0.15.0-a.0.9c476d4b12ba
libbutl 0.15.0-a.0.0ae2c768fc8b
boris-kolpackov commented 2 years ago

[...] is this observation inline with the current implementation?

Yes, that's pretty accurate. A glue buildfile is just a convenience to be able to build packages in the default configuration from the multi-package repository root when using the build system directly. This works fairly well for simple cases where there are no platform-specific packages or similar complications. For the complex cases (platform-specific packages, etc) I think it's best to just remove the glue buildfile since the extra complexity to make it work is not worth the trouble in most cases.

Is there a way to selectively initialize package using the glue buidlfile?

No, bdep doesn't know anything about glue buildfiles. It could theoretically be possible to do it the other way around: for the glue buildfile to only build packages that have been initialized by bdep.

boris-kolpackov commented 2 years ago

@Rookfighter

Here' a bdep ci run with the latest commit [...]

Thanks! So there are 792 builds of which 554 are failures. Looking at the first few it's clear the cause of most of the failures is an attempt to build on platforms that do not apply (like building libimgui-platform-osx on Windows). To reduce the noise it would be helpful to exclude irrelevant platforms as described here: https://build2.org/bpkg/doc/build2-package-manager-manual.xhtml#manifest-package-builds

Swat-SomeBug commented 2 years ago

No, bdep doesn't know anything about glue buildfiles. It could theoretically be possible to do it the other way around: for the glue buildfile to only build packages that have been initialized by bdep.

Would it makes sense to add compilation conditions at the package level? Example:

imgui-platform-osx/imgui/buildfile:
-----------------------------------
./: lib{imgui-platform-osx}: include = ($cxx.target.class == 'macos')

The idea is to disable building the library itself. So even if bdep initialises the package, building itself is prevented at the package level.

boris-kolpackov commented 2 years ago

Would it makes sense to add compilation conditions at the package level?

To me this looks like trying to fix the issue in the wrong place. The correct place is to not bdep-initialize the packages that you don't need. Currently this can be achieved manually (just specify the desired subset with -d). Maybe in the future we can come up with an automatic way, but at this stage this doesn't feel like the best use of our resources.

Klaim commented 2 years ago

In some of my multi-package projects I ended up with a init_configs.sh scripts that would create configs and initialize specific packages depending on the config (for example usually only a few packages can exist in a WASM config). I used that in CI too, so that the CI script ended up being something like

./init_configs.sh
bdep update --all
bdep test --all

(assuming git-bash on windows)

I'm not saying to do this here, just stating my experience. I wonder if it could made easier by some future bdep features, but that's out of scope here.