Pull Request policy thread

General JUCE discussion
1

@adamski suggested we take the discussion about Pull Request policy out of the PACE Purchases JUCE thread and start a new one. So here it is!

4 Likes
JUCE Announces Acquisition by PACE
2

I think it would be nice, if when submitting a pull request / feature request either via GitHub or the forum (I understand forum is prefered and GitHub isn’t really used since GitHub is just a mirror of the actual private JUCE git repository) the JUCE team could respond with ‘Perfect’, ‘Good idea, but the code needs some work’, ‘Ok idea, but low priority’, or ‘Bad idea, we won’t do that’.

We have a fork of JUCE where we have our changes. It would be nice to keep this fork as close to possible the same as the official JUCE if not identical. They what 3rd party JUCE code will work with our JUCE fork, any issues I report I’ll be confident they are actually in JUCE and not something I introduced.

So if you if you have no plans to accept a change, it would be nice to know that so I can plan how to go forward. Either, I can copy / paste the class out of JUCE and make changes and have two similar classes going forward. Or, I can make the change to my JUCE fork and maintain the differences going forward.

10 Likes
3

I’d like to share SR’s perspective as folks who reported and suggested many fixes over the years.

Context

Some fixes we’ve contributed:

  • Countless build problems for various platforms (RTAS, AAX, AUv3, older OS versions, newer OS versions, C++ versions, PowerPC) and Xcode work-arounds (use the more reliable “legacy build system”)
  • AAX bug fixes and features like MIDI
  • VST3-VST2 and AAX-RTAS compatibility (i.e one format can load sessions saved with the other)
  • ScopedNoDenormals
  • AudioProcessor::processBlockBypassed
  • Gain-reduction output for plugins
  • Audio Units support for running in “sand-box” (for GarageBand)
  • Various DAW-specific fixes and work-arounds
  • OpenGL related crashes, deadlocks, bugs, and performance issues
    • One crash was very hard to reproduce, and only triggered on Windows laptops which had Nvidia GPUs in addition to integrated Intel GPUs. We ended up buying a machine with the specific specs to finally succeed in nailing that specific bug down
  • OpenGL using CVDisplayLink on macOS
  • Lots of SVG fixes and missing features
  • Bugs with Drawables and Grid
  • Performance improvements
  • More things that I’ve missed. Consider the above a partial list

We’ve always done an effort to get all the necessary info for a fix, like bisecting exactly which JUCE change broke things, sharing all the relevant details on how to reproduce etc along with suggested code changes to fix it.

We’ve also coded some more significant efforts which JUCE could had benefitted from:

  • Side-chain support for plugins - an extensive effort by @danradix! ROLI did end up adding their own side-chain implementation, which took several iterations until it became stable. IIUC ROLI’s implementation is indeed more complete and allows for multiple side-chain busses, multiple output busses, etc, but we could had also increased the scope of our effort if we had feedback and cooperation for it
  • AAX random-access AudioSuites plugin-side by @ttg (not adopted by JUCE currently)
  • ARA plugin-side together with Celemony (not adopted by JUCE currently)

How a contribution policy affects us

The faster that JUCE incorporates a fix, the less conflict resolution and testing busywork we have to do when merging the branches. Luckily I learned how to use git-mediate which made conflict resolution more effective but it’s still non-negligible work.

To avoid spurious merge conflicts we were already coding our changes according to the JUCE style and doing our best effort to write code how we speculate that Jules would, so often it doesn’t require much more to be worthy for inclusion in JUCE.

Conclusion

Imho it would be really great if JUCE set up any necessary contribution agreements and allow themselves to more effectively get assistance from the community in the form of PRs.

  • Everyone would get to enjoy the community’s fixes faster and less duplication of work will be done.
  • We won’t have to do extensive testing when using our own fix and then again later when using JUCE’s variant (sometimes in several iterations because the first fix doesn’t always work)
  • Less fragmentation will better use the community’s resources and help in testing and reporting any issues with the changes. We still occasionally received some help and reports on our branch but less than if it was in the main JUCE branch
15 Likes
4

PR policies inherently mean more administration overhead. But one way of tackling this might be to integrate the CLA into the juce website, and/or as part of the registration process to the forums? Here is Facebook’s CLA for an idea: https://github.com/facebook/.github/blob/master/CONTRIBUTING.md

To me, all of this seems overkill considering the code will be rewritten to be up to snuff, and that there is no public CI to go through the same motions as the JUCE devs do behind closed doors. Without the overall picture drawn for requirements to know what could make it in for a PR, like following the coding style and say hitting all the marks in CI, posting a PR is more or less futile and the situation won’t change compared to what we have today.

1 Like
5

I expected opinions like the ones you shared, and this is why I tried to “fortify” my comment with the long “context” section.

This is simply incorrect.

There are countless examples but let’s go into a recent one:

When looking at the two commits you’ll notice that it’s exactly the same changes in both. That is - the change incorporated into JUCE are SR’s changes verbatim.

If it’s the exact same code, how could it not be up to snuff at SR’s branch but up to it when in ROLI’s?

1 Like
6

I never called you out, please don’t take my message personally. I’m simply reiterating the JUCE devs’ thoughts, and I do agree with them: consistency with the aesthetic is a good cause.

Not everybody gives the same exact care and consideration so you can’t look at it through only your lens.

2 Likes
7

In our experience, that’s very unusual - even most of the one-liners we get will need tweaking in some way, whether it’s poor naming or just careless spacing. We really only get a handful of people who 100% nail it (and those are generally people like Dave Rowland, Timur, etc)

And sure, it’d be great if we could publish some sort of huge comprehensive document about exactly how to write code that meets our standards, and if everyone who made a PR read it and diligently followed those rules. Brilliant, that’d save us a lot of effort.

But in reality, the majority of PRs we see, even the one-liners, are wrong in some way.

We could spend a long time on each one giving constructive feedback and code-reviewing, and getting the contributor to gradually re-write all the aspects of it that are wrong, until it meets our standard, and then get them to sign a CLA, and then accept the PR.

But that’s an absolute pain, and takes too long, and many people aren’t even motivated to write code that meets our standard, or won’t understand our feedback. And some people won’t want a public record of their coding being criticised.

So we’ve always taken the route of telling everyone “thanks very much, we’ll review this and add it in our own way”. We’re really good at tidying code up quickly, this sidesteps any copyright problems, and if the original contributor is interested in learning from our changes, they can diff it and ask us to explain if necessary.

3 Likes
8

I totally understand where you’re coming from, but there are also plenty of open-source success stories especially in dev-tools, from languages like Python, Rust, D, and Haskell, to frameworks, libraries, and applications. I’ve personally contributed PRs to several open source projects, like the Haskell lens library (1.6k github stars) and code linting tool hlint (1k github stars) (JUCE wins at the github stars count 2k. Not a perfect measure but anyhow I just want to demonstrate that these are not super esoteric projects).

Open-source projects help deal with the problems you mentioned with tooling such as continuous integration running a test-suite, which includes verifying the code’s style using formatting tools like clang-format and linting tools configured with custom rules for the project’s style. Ultimately one finds out that employing such tools improves your personal workflows even when not accounting for their value for external PRs.

While JUCE does stand out from these projects in that it is not “charity based”, but rather there’s a company build around it (similar to QT), I don’t think that this should impede JUCE’s openness to contribution in any way apart from the legalese. Btw I recommend requiring folks to agree to the agreements as a pre-condition before you even start spending resources reviewing their PRs. This is what Google does with its open source projects.

I believe that if you set it up correctly you’ll find that the contribution of the community will be a time saver and well worth the hassle!

6 Likes
9

I’d like to add another positive note here: You would also be helping contributors to become better programmers and better contributors by following such processes. Sure, it would likely be more work (at least in the beginning) than keeping everything within a small team, but IMO would benefit the community and in the long run may lead to gaining some top notch contributors and maintainers.

6 Likes
10

Yeah, we totally get it, we do understand open-source :slight_smile:

But I think there’s an element of luck in finding the right bunch of contributors who happen to click together and find a system which works out well. That hasn’t really happened for juce (yet).

We’re not against the idea of having a community of people doing our work for us! If we did see a stream of great PRs pouring in, or if we had an army of reviewers who’d spend their time reviewing PRs for free, to a standard that satisfies us then that would be a great situation. But engineering that is difficult, and our current system has been working really well for us so far. Maybe in the future things will evolve, who knows.

(And it’s not all happy community fun: I remember speaking to the owner of another well-known open-source, non-commercial project who does rely on user PRs to keep it running, and off-the-record he had a good moan about the group politics that he had to deal with. Sometimes he said he had to accept changes that he knew were crap, and then go back and re-write them sneakily himself to avoid offending the committer.)

5 Likes
11

“The perfect is the enemy of the good.”

Your obsession with minor code style is holding JUCE back. You rather ignore good, constructive code or bug-fixes, because somebody dared not using enough or too many spaces, than swallow your pride or use a tool that does the right formatting for you.

We (reFX) contributed a bug fix that was a major show stopper for ANY semi complex product compiled with JUCE on Windows and our bugfix worked perfectly. It was tested and verified by three developers, but for some unknown reason that wasn’t good enough and had to be rewritten by your team. At least it was taken seriously enough that it ended up in the development branch in a relatively short time frame. So thanks for that.

There are simple documentation issues, off-by-1 errors etc. that could be fixed by a anyone quite easily, but your policies discourage that completely.

7 Likes
12

Indeed. That’s why I opened Fix some typos in juce_core by McMartin · Pull Request #653 · juce-framework/JUCE · GitHub and Fix some typos by McMartin · Pull Request #654 · juce-framework/JUCE · GitHub, which both got “merged” into JUCE by the JUCE team. I never was discouraged.

2 Likes
13

Coding style consistency is important. I’m on Jules’ side in this matter.

3 Likes
14

My obsession with minor (and major) code style is one of the main reasons JUCE has been successful. Consistency is incredibly important for users to navigate and understand the codebase.

Contrast with libraries like ffmpeg which are seething pits of random clashing contributions, where different people re-implement the same thing in different ways and there’s no overall control of the architecture. Lots of people use ffmpeg because they have to, but not because they enjoy it.

Well, maybe ask, and whoever did it can explain why they changed it.

The JUCE team approach is to look at any change and say “OK, assuming this works, could it be done any better?” If not, then nice job, and we’ll probably put it in unchanged.

But if there’s any way we can improve on a bit of code, then of course we will do so. We never take an attitude of saying “OK, this works, and although we can see ways it could be improved, we’re happy to push this mediocre implementation”. If we did that, the overall library quality would slowly decay over time.

5 Likes
15

Can’t this be accomplished with a tool like clang-format though? Or are you talking about variable naming, class design, and control flow?

5 Likes
16

Some aspects of naming (like camelCase, Classes starting with capital letters etc) can also be assisted by tools. Probably also for other aspects that Jules desires like applying a british spell-checker etc

17

Few key-points (imho):

  • The JUCE team are humans. there have been some times where a commit done by JUCE team broke things, introduced bugs, had inconsistent naming convention, etc


  • The discussion/thread is about the policy rather than who’s actually pushing the change to the repo.

What feels that can be improved is the ability to have transparent and organized workflow.

In many cases, PRs on GitHub and forum posts simply decays over time. even one-liners.

To sum it up, It would be nice to have a transparent organized workflow agreed on JUCE with prioritization of topics.

11 Likes
18

I’ve submitted patches to clang-format to get it to approximate the JUCE style, but even the patched version doesn’t get it completely right some (most?) of the time. As an example, I don’t think that any formatter currently exists that would pass the “align = characters in consecutive assignment statements which are semantically similar, but don’t if they’re different” test.

19

Right, and I agree 100% that we should do a better job of replying to each PR and letting the owner know what we think of it, rather than just letting them feel ignored.

11 Likes
20

Thank you Jules for acknowledging that; that was the crux of my comments from the previous thread (JUCE Announces Acquisition by PACE)

I’ve already noticed some very positive improvement in the responsiveness to issues posted here on the forum, with links to the commits that address the various issues.

It would be great to spread this into GitHub PR’s, with a range from “Thanks but we don’t feel this appropriate” to “Here’s the commit we made based on your suggestion”.

5 Likes