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

Cleaning the submodules #111

Merged
merged 1 commit into from
Aug 7, 2024
Merged

Cleaning the submodules #111

merged 1 commit into from
Aug 7, 2024

Conversation

nrichart
Copy link
Collaborator

@nrichart nrichart commented Aug 5, 2024

Description 🤖

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Type of feature/changes 🌲

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Closes #52

@nrichart nrichart linked an issue Aug 5, 2024 that may be closed by this pull request
@nrichart nrichart requested a review from 9and3 August 5, 2024 19:28
@nrichart nrichart self-assigned this Aug 5, 2024
@9and3 9and3 requested a review from Petingo August 6, 2024 08:11
@9and3
Copy link
Collaborator

9and3 commented Aug 6, 2024

Hello @nrichart ! Thank you for finding the time 👐
Small question: I might be mistaken but we use those dependecies like eventtpp right? It seems that getting rid of the might be ok for the build but I wonder about the app..

@nrichart
Copy link
Collaborator Author

nrichart commented Aug 6, 2024

I removed the submodules but the cmake code is still downloading them directly at the configure step

Copy link
Collaborator

@9and3 9and3 left a comment

Choose a reason for hiding this comment

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

Great! Thanks @nrichart for this PR! 👐

@nrichart
Copy link
Collaborator Author

nrichart commented Aug 7, 2024

@9and3 in the PR #109 you added lot of subprojects directly in the deps folder is this part of the fix or is it more a add by mistake ?
Can I remove them and and let you test if this modification is not a regression compared to main. Or would it be possible to add a non regression test ?

@9and3 9and3 merged commit f95dbd7 into main Aug 7, 2024
3 checks passed
@9and3
Copy link
Collaborator

9and3 commented Aug 7, 2024

@nrichart sorry I just merged this one after testing it and it seems to work.

To answer your question: yes I think they were added by mistake.
You can maybe try on a different PR to remvoe them and I will test it if it works correctly. Non-regression tests would be very nice but personally I am under the water now and I won't have time to add them 🐟 , the truth is, we missing a test-suite tout-court :)

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.

CI build is failing
2 participants