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

CMake: misc fixes, using new infra modules #22

Closed
wants to merge 8 commits into from

Conversation

redtide
Copy link
Contributor

@redtide redtide commented Mar 8, 2024

Draft: requires cycfi/infra#19, then change the infra submodule branch to master.
Testing other changes...

@redtide redtide changed the base branch from master to develop March 8, 2024 21:59
@redtide
Copy link
Contributor Author

redtide commented Mar 9, 2024

@djowel I'm also working on an Elements PR and I see we have infra included 2 times there: by both Elements and Artist; can we remove the Elements submodule and use the Artist one?

@djowel
Copy link
Member

djowel commented Mar 9, 2024

@djowel I'm also working on an Elements PR and I see we have infra included 2 times there: by both Elements and Artist; can we remove the Elements submodule and use the Artist one?

Ah makes sense!

@djowel
Copy link
Member

djowel commented Mar 9, 2024

@redtide I think we should first target a feature branch and then merge to develop once stable. There are clients expecting a stable develop branch.

@redtide
Copy link
Contributor Author

redtide commented Mar 9, 2024

Looking at Elements in particular (but also a bit in Artist) after Infra changes there are a lot of things to simplify and also to fix, it's quite a mess (duplicate scripts, different variable names and other little things around), so from my POV it's not about a feature but adapt and fix.

Making a middle way change it's difficult to me, because there are cascading changes to apply that impacts Artist which in turn impacts to Elements, I have no idea how to make this without a straightforward, direct change.

In brief it's not "something nice to have" but "something required". OTOH who is going to test a feature branch which has nothing new to see? I'm not even changing a line of c++ code, it's all about CMake.

@djowel
Copy link
Member

djowel commented Mar 9, 2024

Making a middle way change it's difficult to me, because there are cascading changes to apply that impacts Artist which in turn impacts to Elements, I have no idea how to make this without a straightforward, direct change.

Feature branches are also tested, so I do not know what you mean. I think we are not in the same page. I always test on feature branches for potentially large changes, in multiple repositories. I just make feature branches on all relevant repositories and point each to its dependencies.

In brief it's not "something nice to have" but "something required". OTOH who is going to test a feature branch which has nothing new to see? I'm not even changing a line of c++ code, it's all about CMake.

You cannot guarantee that. CMake changes can also cause disruption. Let's do it in a feature branch please. And please let's not "fix" what is not broken. I am OK for an incremental upgrade, but not for a wholesale change.

No argument, please ;-)

@djowel
Copy link
Member

djowel commented Mar 9, 2024

feature branch which has nothing new to see

The binaries. Those are not vetted yet.

- use optional empty source_groups
- specify the detail file, CMake can't add directories without using file(GLOB[_RECURSE])
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.

2 participants