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

Remove unnecessary submodules from tools directory #2196

Merged
merged 1 commit into from
Dec 29, 2021

Conversation

softins
Copy link
Member

@softins softins commented Dec 28, 2021

Git submodules are intended for bringing in a copy of another repo that is needed for building the project. They are not for general reference to other projects of interest.

This commit removes those unneeded submodules, and instead adds a text file listing those repos, with a link to each and a brief description.

Context: Fixes an issue?

The CI build pulls in all submodules, most of which are currently unnecessary and sometimes fail.

Does this change need documentation? What needs to be documented and how?

No documentation needed

Status of this Pull Request

Ready

What is missing until this pull request can be merged?

Discussion and approval.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

Git submodules are intended for bringing in a copy of another repo
that is needed for building the project. They are not for general
reference to other projects of interest.

This commit removes those unneeded submodules, and instead adds a text
file listing those repos, with a link to each and a brief description.
@softins
Copy link
Member Author

softins commented Dec 28, 2021

Windows build seems to be failing currently, due to an unrelated reason.

Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Looks good in general. How big is the speed-up?

@@ -0,0 +1,49 @@
# Other projects related to Jamulus
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you want to add this to the main repo? Maybe it would be better to have it as a post in the community KB on the website.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe. I just didn't want to completely remove information from the repo, so I created this document as somewhere to put it. If we moved it to the web site and removed it from here, people cloning the repo might not know the information is available to be found elsewhere. I don't always find things easy to locate on the website.

Copy link
Member

Choose a reason for hiding this comment

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

I don't always find things easy to locate on the website.

Some advanced stuff is quite hidden… probably worth re-organising the site?

Personally, I‘d prefer to have documentation on the website, but it’s up to you where to put it.

Asking for another opinion:
@gilgongo should we move this document to the website?

Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to have it them on the website. Can't immediately think where though - perhaps as page linked out from the client and server manuals?

Copy link
Member

Choose a reason for hiding this comment

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

Community KB?

Copy link
Member

Choose a reason for hiding this comment

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

I suppose since these projects are a) subject to change out of our control and b) not core core information about Jamulus, then they would be a candidate for the KB, yes.

Copy link
Collaborator

@pljones pljones left a comment

Choose a reason for hiding this comment

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

Looks good.

@softins
Copy link
Member Author

softins commented Dec 29, 2021

Looks good in general. How big is the speed-up?

No difference in the actual execution. It does avoid some unnecessary fetching of submodules in the CI build. It was the failure of one of those fetches recently that prompted me to look at it.

@ann0see ann0see self-requested a review December 29, 2021 12:16
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Going to approve it as is. The documentation question is not blocking

@softins softins merged commit 26a896d into jamulussoftware:master Dec 29, 2021
@softins softins deleted the remove-submodules branch January 11, 2022 12:16
@gilgongo gilgongo added this to the 3.8.2 milestone Jan 17, 2022
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.

4 participants