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

module: build failure #43

Closed
gucio321 opened this issue Dec 3, 2022 · 23 comments
Closed

module: build failure #43

gucio321 opened this issue Dec 3, 2022 · 23 comments
Labels
bug Something isn't working

Comments

@gucio321
Copy link
Collaborator

gucio321 commented Dec 3, 2022

Hi, I'm having trouble building the latest code

cd examples
go run .
# github.com/AllenDang/cimgui-go
In file included from ./cimplot_wrapper.h:3,
                 from ./cimplot_structs_accessor.h:3,
                 from ../cimplot_funcs.go:4:
./cimplot/cimplot.h:7:10: fatal error: cimgui.h: No such file or directory
    7 | #include "cimgui.h"
      |          ^~~~~~~~~~
compilation terminated.
@gucio321 gucio321 added the investigating Searching for origins of the issue. label Dec 3, 2022
@gucio321
Copy link
Collaborator Author

gucio321 commented Dec 3, 2022

I'd blame #18

@gucio321 gucio321 changed the title build: package seems to be broken and doesn't build anymore? module: build failure Dec 3, 2022
@gucio321 gucio321 added the bug Something isn't working label Dec 3, 2022
@gucio321
Copy link
Collaborator Author

gucio321 commented Dec 3, 2022

the reason is, that both: cimplot.* and cimgui.* should be in the same directory OR the path to cimgui.h in cimplot generator should be set manually. Otherwise, cimplot.h searches for cimgui.h in its directory (in our case /cimplot/) and there is no such a file ofc.
becore #18 the line in cimplot.h was

#include "../cimgui/cimgui.h"

and now it is

#include "cimgui.h"

thats the reason of this failure

@eliasdaler
Copy link
Contributor

The inclusion of cimplot by default here rubs me in the wrong way anyway, it introduces a ton of complexity and ImPlot is not a part of Dear ImGui anyway...
So it might be better to do this:

  1. Remove ImPlot temporarily
  2. Add it again properly and make it work with git submodules (and ideally move it to a different package, because a lot of people won't need it)

@eliasdaler
Copy link
Contributor

eliasdaler commented Dec 3, 2022

Well, I almost got it to work:

diff --git a/backend.cpp b/backend.cpp
index 556bda4..9913e09 100644
--- a/backend.cpp
+++ b/backend.cpp
@@ -4,8 +4,8 @@
 #define CIMGUI_USE_OPENGL3
 
 #include "backend.h"
-#include "cimgui/cimgui.h"
-#include "cimgui/cimgui_impl.h"
+#include "cimgui.h"
+#include "cimgui_impl.h"
 #include "thirdparty/glfw/include/GLFW/glfw3.h" // Will drag system OpenGL headers
 
 // [Win32] Our example includes a copy of glfw3.lib pre-compiled with VS2010 to
diff --git a/cimplot_funcs.go b/cimplot_funcs.go
index 22ad6fb..1a6e66e 100644
--- a/cimplot_funcs.go
+++ b/cimplot_funcs.go
@@ -1,5 +1,7 @@
 package cimgui
 
+// #cgo CFLAGS: -Icimgui -Icimgui/generator/output
+// #cgo CXXFLAGS: -Icimgui -Icimgui/generator/output
 // #include "extra_types.h"
 // #include "cimplot_structs_accessor.h"
 // #include "cimplot_wrapper.h"

Now, I get these errors:

# github.com/AllenDang/cimgui-go
./cimgui_funcs.go:426:145: cannot convert glyph_ranges (variable of type *ImWchar) to type *_Ctype_ushort
./cimgui_funcs.go:436:182: cannot convert glyph_ranges (variable of type *ImWchar) to type *_Ctype_ushort
./cimgui_funcs.go:443:195: cannot convert glyph_ranges (variable of type *ImWchar) to type *_Ctype_ushort
./cimgui_funcs.go:450:163: cannot convert glyph_ranges (variable of type *ImWchar) to type *_Ctype_ushort
./cimgui_funcs.go:488:20: cannot convert func() *_Ctype_ImWchar {…}() (value of type *_Ctype_ushort) to type *ImWchar
./cimgui_funcs.go:492:20: cannot convert func() *_Ctype_ImWchar {…}() (value of type *_Ctype_ushort) to type *ImWchar
./cimgui_funcs.go:496:20: cannot convert func() *_Ctype_ImWchar {…}() (value of type *_Ctype_ushort) to type *ImWchar
./cimgui_funcs.go:500:20: cannot convert func() *_Ctype_ImWchar {…}() (value of type *_Ctype_ushort) to type *ImWchar
./cimgui_funcs.go:504:20: cannot convert func() *_Ctype_ImWchar {…}() (value of type *_Ctype_ushort) to type *ImWchar
./cimgui_funcs.go:508:20: cannot convert func() *_Ctype_ImWchar {…}() (value of type *_Ctype_ushort) to type *ImWchar
./cimgui_funcs.go:508:20: too many errors

I don't know how to fix them yet, but it finds all the headers it needs at least...
I'll debug it tomorrow.

@eliasdaler
Copy link
Contributor

Welp, I found a bag of worms...

See this CMakeLists.txt. It wasn't even a "real" CMakeLists.txt from cimgui. It was an amalgamation of cimgui, glfw backend and cimplot
So, by linking to "./lib/***", we were linking to it and not to real cimgui.a which you could build from scratch.

This is wrong on many levels. There shouldn't be any "prebuilt" libraries in projects using CGo. They should be compiled from source. Dependency on GLFW backend and cimplot adds another bag of problems I'm not even sure where to start.

I'll try to fix all this, but I'd rather start from removing glfw and cimplot dependency, making "cimgui" wrapper correct and then adding things back one by one.

@AllenDang
Copy link
Owner

AllenDang commented Dec 4, 2022

I think we should provide prebluit binaries. We shouldnt assume everyone has proper c/cpp compiles installed.
This repo should work with only go is installed.
Thats why i use build actions to compile them.

@eliasdaler
Copy link
Contributor

eliasdaler commented Dec 4, 2022

Yeah, I did some investigations and building libraries during "go get" is not that great in all cases.
But, I'm also working on fixing a lot of stuff here.

For now, it only builds on Linux (only 1 precompiled lib here). I've temporarily removed GLFW backend and implot to make things easier. I will add them later in the way that will not require libimgui.a to NOT contain implot functions and I want cimgui-go to not depend on GLFW in any way.

Like I previously said, I don't use GLFW. Some people might want to use SDL/raylib. Current cimgui-go forces users to embed GLFW code into their binaries. This is not great.

Also, turns out git submodules cannot be used. When I tried to import cimgui-go using "go get" for ebiten-imgui, I got errors about cimgui.h not being there because "go get" doesn't fetch submodules. (running example)

So, ideally we should ship all needed headers inside cimgui-go. But I think we should do some form of "git submodule" system where we'll have a file like:

cimgui=https://github.com/cimgui/cimgui@8e625e95fdf469e105826cc22cb7665949f3aa21

and have a GitHub action named "update cimgui" which will automatically pull the corresponding version into the repo, build all libraries and commit it. We should always know which version of cimgui was used in a specific commit/build of all libraries.

For cimplot and GLFW backend... Well, I need to think. I can deal with cimplot being a part of cimgui which I can't get rid of, but having to depend on GLFW - this I cannot accept.

@sonoro1234
Copy link

sonoro1234 commented Dec 4, 2022

My two cents:

You need to create a new CMakeLists.txt ( as done in https://github.com/sonoro1234/LuaJIT-ImGui) which picks whatever you want to use (cimgui, cimplot and glfw backend for example). cimgui and cimplot are ok as submodules but for glfw you can use find_package or FetchContent (as done in https://github.com/cimgui/cimgui/blob/68483775b3d7594eb14d296ec002ac11683882c9/backend_test/example_glfw_opengl3/CMakeLists.txt#L65)
I guess some go machinery will need to be added to this. (with CGo include paths setted)

Then you could use github Releases (https://github.com/AllenDang/cimgui-go/releases) if you wish to provide binaries so that user doesn`t need to compile. (Althought user will be able to compile if he/she wants to do it)

@sonoro1234
Copy link

Also: In the cmake file you can have a variable USE_XXX_BACKEND to customize build for using GLFW, SDL or any other

@eliasdaler
Copy link
Contributor

My two cents:

You need to create a new CMakeLists.txt ( as done in https://github.com/sonoro1234/LuaJIT-ImGui) which picks whatever you want to use (cimgui, cimplot and glfw backend for example). cimgui and cimplot are ok as submodules but for glfw you can use find_package or FetchContent (as done in https://github.com/cimgui/cimgui/blob/68483775b3d7594eb14d296ec002ac11683882c9/backend_test/example_glfw_opengl3/CMakeLists.txt#L65) I guess some go machinery will need to be added to this. (with CGo include paths setted)

Then you could use github Releases (https://github.com/AllenDang/cimgui-go/releases) if you wish to provide binaries so that user doesn`t need to compile. (Althought user will be able to compile if he/she wants to do it)

Hello. Thanks for the tips. However, users of Go libraries typically don’t ever compile anything or use CMake (unless go lib includes compilation via cgo commands, e.g. go-gl/glfw). The only way to not require a C compiler is to use cgo and compile the lib on the fly. This is also OK for me. But Go lib users won’t ever use CMake… but we could usr it internally, of course.

@gucio321
Copy link
Collaborator Author

gucio321 commented Dec 5, 2022

Yeah, I did some investigations and building libraries during "go get" is not that great in all cases.
But, I'm also working on fixing a lot of stuff here.

@eliasdaler let me know if I can help you somehow

@eliasdaler
Copy link
Contributor

eliasdaler commented Dec 5, 2022

@eliasdaler let me know if I can help you somehow

Here are some steps that I propose we do for now.

  1. Stop using git submodules - don't revert the patch but instead remove git submodule usage and commit repo contents to the dirs
  2. Create a file which will look like this (let's name it .dependencies_metadata and place it in the root of the repo):
cimgui=github.com/cimgui/cimgui@68483775b3d7594eb14d296ec002ac11683882c9
cimplot=...

This file will be used as a documentation for us (to know which version of cimgui, cimplot and glfw we have).
Later we can parse it and make a GitHub action to remove "cimgui" dir, clone a new version at a specified commit and commit it to the repo.

  1. Make a CMake/just build for building cimgui+cimplot static lib (should come as one lib). Ideally you should use CMake because with it you'll be able to just do "add_subdirectory" for "cimgui" and "cimplot" and make a new static library out of them. If you're not experienced with CMake, I'll do it myself later. GLFW build is not needed, we'll remove dependency on it soon.
  2. Change existing "build-lib-*" workflows to use this build script to build and commit static libs to the repo. The workflow should be ran manually, but it would be better if it would build for all platforms at once so that you don't need to run 3 workflows.

I won't have much time to work on cimgui-go myself this week, but I'll be able to review PRs.
I think that you can make cimgui-go compilable again if you add cgo directives with "-I" just like I did in my branch. The problem I had with ImWchar were caused by IMGUI_USE_WCHAR32 not being defined. I still didn't figure out how to force it to be enabled. Might work on it later - let's go without it and change ImWchar to ushort for now like I did here.

@eliasdaler
Copy link
Contributor

eliasdaler commented Dec 5, 2022

Actually, ushort vs uint thing is a bit more complicated... By default cimgui generates the files without IMGUI_USE_WCHAR32... So if we want unicode, we need to run generators ourselves. But the results should not go to "cimgui" and "cimplot" directories - they should be left as-is.

Instead, we should place them in some "prebuilt" directory, e.g.

"prebuilt/include/cimgui.h"
"prebuilt/lib/linux/amd64/libimgui.a"

... and later configure cgo's search paths to use these files when compiling Go.

Well, this is pretty complicated...

@gucio321
Copy link
Collaborator Author

gucio321 commented Dec 6, 2022

I think that you can make cimgui-go compilable again if you add cgo directives with "-I" just like I did in my branch

sadly, it complains about cimgui_impl.h form backend.go

Stop using git submodules - don't revert the patch but instead remove git submodule usage and commit repo contents to the dirs
Create a file which will look like this (let's name it .dependencies_metadata and place it in the root of the repo):

well, but I don't actually understand, why to stop using git submodules, since you propose exactly the same solution but without git submodules.
Also, why wouldn't I revert my PR? It'd be better for storage reasons, since (as far as I understand git 😄) when "reverting" git just says "do the same as in these commits, but backwords".

@eliasdaler
Copy link
Contributor

I think that you can make cimgui-go compilable again if you add cgo directives with "-I" just like I did in my branch

sadly, it complains about cimgui_impl.h form backend.go

Did you add -Icimgui/generator/output? It is probably caused by this.

Stop using git submodules - don't revert the patch but instead remove git submodule usage and commit repo contents to the dirs
Create a file which will look like this (let's name it .dependencies_metadata and place it in the root of the repo):

well, but I don't actually understand, why to stop using git submodules, since you propose exactly the same solution but without git submodules. Also, why wouldn't I revert my PR? It'd be better for storage reasons, since (as far as I understand git smile) when "reverting" git just says "do the same as in these commits, but backwords".

Hmm, yeah, I thought there were other improvements to CI, but there weren't. However: the previously commited 3rd party directories were not versioned properly and contained modifications to CMakeLists.txt of cimgui. So yeah, we can revert and fix that, or we could just remove submodules and add 3rd party libs properly this time...

Storage reasons - doesn't matter even if it actually did it like this (but I don't think it does). We'll need to add a different sources for 3rd party libs anyway...

@eliasdaler
Copy link
Contributor

Okay, I'll try making something later today to make exactly what I want.

  1. Remove glfw dependency and backend - temporarily provide ebiten-imgui backend as a substitute.
  2. Remove submodule usage
  3. Properly commit cimgui and cimplot to the repo.

@Chillance
Copy link

Chillance commented Dec 9, 2022

So, while this is being fixed, I suppose I can use https://github.com/AllenDang/giu in the meantime. Also, can someone elaborate on it? I assume this cimgui-go will be taking over as soon as more things are in place? I really like that cimgui-go can essentially generate wrapper code in Go based on C/C++ header files. It's nice way to keep things up to date, but currently it's not clear which one I should use as both seems to be active...

@gucio321
Copy link
Collaborator Author

@eliasdaler any updates?
I think, we should revert #18 in order to, at least, make the repo compilable and continue working on another issues.
We can continue work on submodules somewhere else.

@gucio321
Copy link
Collaborator Author

So, while this is being fixed, I suppose I can use https://github.com/AllenDang/giu in the meantime.

Sure @Chillance! GIU works like a charm!

Also, can someone elaborate on it? I assume this cimgui-go will be taking over as soon as more things are in place? I really like that cimgui-go can essentially generate wrapper code in Go based on C/C++ header files. It's nice way to keep things up to date, but currently it's not clear which one I should use as both seems to be active...

cimgui-go is just a wraper of imgui, while giu is a wraper of wraper 😄
it just provides easier in use set of types. So if you was working with C++ version of imgui, You may want to use cimgui-go, else I'd recommend you take a look on giu since it is easier to understand

Also, giu is going to migrate to use this repo in future

@andraantariksa
Copy link

@eliasdaler any updates? I think, we should revert #18 in order to, at least, make the repo compilable and continue working on another issues. We can continue work on submodules somewhere else.

I think you should do that in the meantime. I really want to use this package so badly, but I just cant compile it.

@eliasdaler
Copy link
Contributor

@eliasdaler any updates? I think, we should revert #18 in order to, at least, make the repo compilable and continue working on another issues. We can continue work on submodules somewhere else.

Reverted.

@gucio321
Copy link
Collaborator Author

@andraantariksa try now, it should work

@eliasdaler
Copy link
Contributor

The last two reverts about git submodules should have helped.
If you have new build issues feel free to re-open or create a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants