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

feat: rnnoise and ringbuf as submodules rather than copied libraries #268

Merged

Conversation

TheDukeofErl
Copy link
Collaborator

This PR migrates the dependencies that are on github to be submodules to lower the overall code audit needs over time.

This isn't something that has to be approved now but I think is worth considering in order to simplify the maintenance of the code. Once the current audit is finished this can then be tested and merged.

When the time comes to update the README with information on the repository again, it would be a good idea to include instructions for cloning the repo git clone --recurse-submodules https://github.com/noisetorch/NoiseTorch.git and updating the repo git submodule update --init --recursive.

Note that the ladspa files themselves aren't as simple to deal with in terms of doing this: I was able to find some of the involved files in other repos but wasn't able to find the full dependency so that may be something that just has to remain maintained by the noisetorch organization.

@TheDukeofErl TheDukeofErl changed the title Feature/rnnoise ringbuf submodule feat: rnnoise and ringbuf as submodules rather than copied libraries May 20, 2022
@OldiLo OldiLo mentioned this pull request May 20, 2022
@ZyanKLee
Copy link
Contributor

I personally like the idea of removing external dependencies from the projects codebase.

To ensure the supply chain, I would suggest to fork those projects into the org instead of using them directly.

That way we would on the one hand stay in control about which state of those projects will be used during build (by also using them as submodules) and also make sure deleted or otherwise impaired projects will not be a problem for us.

@TheDukeofErl
Copy link
Collaborator Author

I think that's a reasonable course of action. I initially took this approach, more or less copying how Valve handled it with Proton by just using the repositories directly.

Once the noisetorch org forks rnnoise and c-ringbuf I'll update the PR. Taking that approach, a better and more consistent course can even be taken with ladspa, which would be great.

@ZyanKLee
Copy link
Contributor

Once the noisetorch org forks rnnoise and c-ringbuf I'll update the PR. Taking that approach, a better and more consistent course can even be taken with ladspa, which would be great.

I forked both projects.

@TheDukeofErl
Copy link
Collaborator Author

Thanks, I've gotten the submodules pointed at the noisetorch forks.

@ZyanKLee ZyanKLee added the refactoring Rewriting or improving existing behaviour label May 21, 2022
Copy link
Contributor

@ZyanKLee ZyanKLee left a comment

Choose a reason for hiding this comment

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

This basically LGTM. Just one small issue I had, that has a suggested fix in @OldiLo's PR #269 .

c/ladspa/Makefile Outdated Show resolved Hide resolved
Co-authored-by: OldiLo <58697558+OldiLo@users.noreply.github.com>
@OldiLo
Copy link
Contributor

OldiLo commented May 21, 2022

Good

EDIT: and please, remove the vendor folder

@TheDukeofErl
Copy link
Collaborator Author

EDIT: and please, remove the vendor folder

I think it's best to have that in a separate, specific to that task PR, for a couple of reasons, the first being that keeping this PR focused on just these dependencies makes it easily trackable/reversible (if necessary), the second being that there needs to be an open discussion over the removal of the vendor folder given the previous pushback against that.

@ZyanKLee ZyanKLee merged commit 9f5cfdf into noisetorch:master May 23, 2022
@TheDukeofErl TheDukeofErl deleted the feature/rnnoise-ringbuf-submodule branch May 24, 2022 15:25
@ZyanKLee ZyanKLee added this to the v0.12.0 milestone May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Rewriting or improving existing behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants