Skip to content

Conversation

@gucio321
Copy link
Collaborator

Hi there!
there is a technology called git submodules that allows to put a git repository inside another one.
I think that instead Drop source code of imgui to cimgui/imgui you can just execute git submodule update --init --recursive
it has several adventages:

  • don't need to search what imgui and implot exactly are
  • clone them with one command
  • exact conrol over version (submodule has specified the version that could be set somewhere - i can't remember where 😄 )

@AllenDang
Copy link
Owner

Thanks! The git action to build also need to change.

@eliasdaler
Copy link
Contributor

The disadvantages I see here is that you'll need to generate cimgui outputs manually.

It's possible to add a GitHub action which will generate and commit outputs on submodule version update. Alternatively, we can add cimgui generation as a build step.

@gucio321
Copy link
Collaborator Author

sure, I can write some workflow

@gucio321
Copy link
Collaborator Author

here is how Am I going to update submodules in workflow ;-)

@AllenDang
Copy link
Owner

@gucio321 Check the ".github/workflows", run them via "Actions" for win/mac/linux, make sure they could be compiled successfully.

@gucio321
Copy link
Collaborator Author

yah, I'll do that as soon as I can make the current version working ;-)

@gucio321
Copy link
Collaborator Author

@AllenDang I've a question: do I need to regenerate the whole binding after updating dependencies?

@gucio321
Copy link
Collaborator Author

ok, it seems to work now
@AllenDang is this workflow ok?
it updates submodules and regenerates binding. if regeneration crashes it'll exit with error
The workflow runs every week on monday

@eliasdaler
Copy link
Contributor

@gucio321, can you please manually run luajit ./generator.lua gcc "comments internal" glfw opengl3 -DIMGUI_DISABLE_OBSOLETE_FUNCTIONS -DIMGUI_USE_WCHAR32 for cimgui generation instead of generator.sh?

Will be useful for #27

@gucio321
Copy link
Collaborator Author

@eliasdaler like this? 40084d5

Don't you think that reffering to sh script would be better than hard-codding a command in workflow?

Copy link
Contributor

@eliasdaler eliasdaler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine overall, I'll test how well it works with my code a bit later today.

@eliasdaler
Copy link
Contributor

@gucio321, I also think that justfile needs some adjustments. It looks like it's currently not possible to do "just gencode_cimgui" locally and build new *.go files
I think that ideally justfile needs to be changed to be able to generate cimgui/cimplot outputs by just running "just gencode_cimgui" and "just gencode_cimplot"

The generation should ideally generate output files to some place outside of cimgui/cimplot directories. These directories should also be added to this repo's .gitignore and not commited.

This will also make GitHub workflows easier as you'll be able to just run "just gencode_cimlot" and "just gencode_cimgui".

It would also be great to add ability to detect that user didn't clone submodules before running "just " by checking for some file's existence inside cimgui/cimplot dirs (e.g. README.md)

gucio321 added a commit to gucio321/cimgui-go that referenced this pull request Nov 22, 2022
@eliasdaler
Copy link
Contributor

@gucio321 any updates on this? It's almost ready, but needs to work locally :)

@gucio321
Copy link
Collaborator Author

oh, you're right, there is something wrong with justfile...

let me take a look

@gucio321
Copy link
Collaborator Author

oh, nvm, i had corrupted submodules 😄

@gucio321 any updates on this? It's almost ready, but needs to work locally :)

so @eliasdaler whot do you think, should be done here? IMO this is complete. it works locally for me

@eliasdaler
Copy link
Contributor

I'll try it a bit later today. :)

@eliasdaler
Copy link
Contributor

eliasdaler commented Nov 30, 2022

Okay, it works for me locally, that's good.
But I have two questions:

  1. How exactly does that "auto update dependencies" task work? On which branches will it run? It only needs to run on "main" branch, imo. Plus, I think it can run once a month. Dear ImGui do a release every week. And if it does some hotfixes, we can manually do the update.
  2. It looks like you can drop "sh generator.sh" in workflows because the both cimgui and cimplot already have bindings generated inside of them

@gucio321
Copy link
Collaborator Author

How exactly does that "auto update dependencies" task work? On which branches will it run? It only needs to run on "main" branch, imo. Plus, I think it can run once a month. Dear ImGui do a release every week. And if it does some hotfixes, we can manually do the update.

it, every week, updates cimgui/cimplot deps and regenerates framework, it will run only on master (main)

It looks like you can drop "sh generator.sh" in workflows because the both cimgui and cimplot already have bindings generated inside of them

it does instruction from readme tbh 😄
I can remove this step, but, imo, we can regenerate C binding to make sure everything is correct there

@eliasdaler
Copy link
Contributor

If you don't want to remove C binding generation, you should make it so that C binding is generated outside of submodule director. Otherwise, you might run into problems if you change files inside submodule directories

@gucio321
Copy link
Collaborator Author

gucio321 commented Dec 1, 2022

Otherwise, you might run into problems if you change files inside submodule directories

ok, fair enough, removing step responsible for regenerating c binding

@gucio321
Copy link
Collaborator Author

gucio321 commented Dec 1, 2022

@eliasdaler done, now Update Dependencies only runs just

@AllenDang
Copy link
Owner

@gucio321 @eliasdaler Seems ready to merge?

@eliasdaler
Copy link
Contributor

@AllenDang LGTM

@AllenDang
Copy link
Owner

@AllenDang LGTM

May I ask what LGTM stands for...

@eliasdaler
Copy link
Contributor

@AllenDang LGTM

May I ask what LGTM stands for...

"Looks good to me" -> let's get this merged :D

@AllenDang AllenDang merged commit 47f2614 into AllenDang:main Dec 1, 2022
This was referenced Dec 3, 2022
@gucio321 gucio321 deleted the use-submodules branch December 3, 2022 17:11
@gucio321 gucio321 restored the use-submodules branch December 3, 2022 17:12
gucio321 added a commit to gucio321/cimgui-go that referenced this pull request Dec 3, 2022
eliasdaler pushed a commit that referenced this pull request Dec 10, 2022
This reverts commit 47f2614, reversing
changes made to c489688.

"go get" doesn't clone submodules recursively which means that cimgui-go can't be imported via "go get" in other repos.
@gucio321 gucio321 deleted the use-submodules branch November 20, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants