provides an excellent example of why commit messages matter. This is more than 1000 lines diff to the build system (and thus potentially very dangerous because of possible supply chain attacks) which has the commit message of
"CI: Update build scripts and Github actions workflow"
Never say what you ware doing (it's obvious this commit is updating build scripts). Say why you are doing it, especially when your commit feels unrelated or at most very tangentially related to the objective of your PR.
> Never say what you ware doing (it's obvious this commit is updating build scripts)
DEFINITELY say what you are doing. And then say why.
This blog post is a good description of the most prevalent convention in how to write good and useful messages (e.g. this is what the Linux Kernel does and given the history of git there's some flow down): https://cbea.ms/git-commit/
The key thing that means you should start with what is having a short one-line summary of what you're doing so you can look at a simple log and see which commit you're interested in.
We have a rule internally that a PR can’t be more than 1,000 loc. I personally find a 1,000 loc PR almost unreviewable. It takes me multiple days and a stack of notes, and still almost always ends up with effects I didn’t anticipate.
Trying to understand the changes and their full implications is nearly impossible. 100 loc is a better top end imho.
Question from a naive kiddo; When the PRs are this big, what are the steps taken to "get it in" the master ? Just blindly accept based on CD/CI jobs, or break it down to walk through every change ? "Ref: supply chain attacks"
Just to chip in what should the commit message say: it should express the intent of the change and the reason of the change (latter could be reference to ticket). I don't think expressing how the intended change is achieved is important, that's what the diff is. If it's not clear in the diff how the intended change is achieved, try adding some comments.
Never say what you ware doing (it's obvious this commit is updating build scripts). Say why you are doing it
Don’t say what you were doing, say what the commit does. A good commit message completes the sentence “when merged this will …”.
Adding a more detailed description on the second line including why things were done is also not bad, but I have a strong opinion that design notes do not belong in the commit log but in code comments or markdown files that are in the repo, so that they can be found more easily and updated.
Note that those commits seem to have been added by an OBS contributor on top of Apple's patch. The fact they touch the entire build system, in a PR that is 100% macOS focused, is much more concerning than the commit messages.
The weird thing to me is that it's not just lower CPU/GPU for OBS, but lower GPU for the game. Maybe just a coincidence, since the CPU fluctuates so much between the two.
It's a bit strange to open a pull request from "Developer-Ecosystem-Engineering" instead of an individual account. I wonder if every interaction from this account has to be approved by a board or something.
Don't Apple have a policy that their developers aren't supposed to contribute to Open Source without explicit permission (https://news.ycombinator.com/item?id=22935263)? I imagine this is the result of that policy - if Apple wants to contribute to something as a business there isn't really anyone who would be able to use their individual account to do it without lots of people questioning why that person can when others can't.
Could be related to the decision to remove individual names from the product about boxes back when Jobs came back to better control leaking of internal information and cut down on poaching. I would think that Apple does not want anyone to know who is working on the screen capture APIs as this PR would probably come from one member of that group
"When Steve came back, one of his company-wide edicts was that the names of individuals must be removed from about boxes."
HP tried to do something similar when working with openstack. It was done for very bureaucratic reasons / company visibility. (Author was preserved, commit was by something like [email protected]) A slightly misguided idea by people who don't normally work with big open source. It was reverted since it messed with project statistics.
From experience in a big company (not Apple) likely direct manager then senior manager then legal department. The justification and hoop jumping would take much more effort than the coding.
Well, given the amount of publicity this PR is getting it probably makes sense to not just let a random software engineer post whatever they feel like.
For those of you who have contributed to a Github (or similar) project in your capacity as an employee, do you use a work account separate from your own?
It's actually some of the best software I've used for streaming. Also, the feature to record a browser tab is a life saver when I'm double booked and don't want to miss content from a meeting.
< The __MAC_10_15 define may not exist on devices running < 10.15, resulting in this section not being appropriately avoided during compilation. Swapping to 101500 should work for all versions of macOS.
So they are mindful of people with old Macs. This is good!
I find this PR an interesting opportunity to discuss different types of documentation.
I've internalized that there's a difference between the PR description, commit messages, and code comments. For me there's different intended audiences for all three, and their contents should thus be shaped accordingly. This PR basically combines all three into one and uses that as the commit message, and I'm not sure of what to make of it. Is this good practice?
Personally I find the contents of the "#### Summary of Changes" list to be super-neat and helpful. But should these not be more helpful as comments inside the code? On the other hand, it does explain what the code is doing at this snapshot in time: which makes it more precise than comments in the code -- since future changes might eventually make those comments incorrect.
> On the other hand, it does explain what the code is doing at this snapshot in time: which makes it more precise than comments in the code -- since future changes might eventually make those comments incorrect.
If changes are made to the code, the comments should be updated accordingly. Getting from code to a PR text is a non-obvious and difficult process and when parts of the code are updated, the trail is easily lost.
Personally I like for the dev to do little commits that document each change to a file and then when the branch is merged in you do a squash merge and each commit msg becomes a bullet point.
Now I hope they improve the situation of virtual cameras. Now you need to code sign all kinds of applications in order for them to recognize your applications, which often breaks those applications.
If anyone watches "Meet Kevin" youtube channel, you would know the bad publicity Apple gets with him attempting to stream on his relatively new Mac Pro system. Any effort to improve that capability would be in Apple's best interest. Streaming is not going away.
I want to know if they’re using Rosetta on their M1 test machine or running it natively. Hopefully this patch leads to a proper darwin/arm64 release so I don’t have to use this weird fork[0] with a private GitHub Action to get a native build. The Rosetta version is considerably slower.
> I want to know if they’re using Rosetta on their M1 test machine or running it natively
Natively.
There's an open PR[0], #5155, to get OBS native on Apple Silicon. That's also what your weird fork uses.
Apple's PR is on top of that work: you'll see that there are 10 or so initial commits. Thus #5155 will need to be merged first, before Apple's.
#5155 is a huge PR and has been open for 5 months. Its release will also necessitate OBS get an M1 testing solution figured out. Here's a quote[2] from an OBS team member two days ago:
> [This PR is] top of our priority list once we release 27.2. We're keenly aware of how badly people want a native Apple Silicon build, and I can assure you that we will definitely be publishing official test builds of OBS with M1 support as soon as we are able.
I would nevertheless expect for all of this to take quite a while.
I wonder if this will provide a way to hide the yellow dot that appears in the corner (to indicate mic access). It is a great idea in most cases, but quite annoying to see on screen recordings.
Maybe OBS will work on my Mac now. I struggled to get it working, everything recording as I wanted. Thought it was just "crappy open source" stuff, so gave up.
Some time later, I tried OBS on Windows, and it just worked, first time, no fiddling required. And I realised the problem was on the Mac side, not Obs.
In the PR description, if you expand the sections "Baldur’s Gate 3 Demo", "Shadow of the Tomb Raider", etc. it shows the performance improvement of capturing the respective games. Looks like a very significant improvement.
Should this allow OBS to not capture the actual OBS window itself? I'd think it would be nice to eliminate that from what is captured, for those poor people who only have a single monitor. :)
How does one go about reviewing such a PR? If the PR consists of smaller commits then it would probably be easier. But if a large commit is made, then what would be the flow to review it?
[+] [-] pilif|4 years ago|reply
https://github.com/obsproject/obs-studio/pull/5875/commits/8...
provides an excellent example of why commit messages matter. This is more than 1000 lines diff to the build system (and thus potentially very dangerous because of possible supply chain attacks) which has the commit message of
"CI: Update build scripts and Github actions workflow"
Never say what you ware doing (it's obvious this commit is updating build scripts). Say why you are doing it, especially when your commit feels unrelated or at most very tangentially related to the objective of your PR.
[+] [-] rkangel|4 years ago|reply
DEFINITELY say what you are doing. And then say why.
This blog post is a good description of the most prevalent convention in how to write good and useful messages (e.g. this is what the Linux Kernel does and given the history of git there's some flow down): https://cbea.ms/git-commit/
The key thing that means you should start with what is having a short one-line summary of what you're doing so you can look at a simple log and see which commit you're interested in.
[+] [-] n3_|4 years ago|reply
[+] [-] donatj|4 years ago|reply
Trying to understand the changes and their full implications is nearly impossible. 100 loc is a better top end imho.
[+] [-] rlt|4 years ago|reply
Also HN: no, not like that!
[+] [-] ff133|4 years ago|reply
[+] [-] michaelt|4 years ago|reply
[1] https://github.com/obsproject/obs-studio/pull/5875
[+] [-] steerablesafe|4 years ago|reply
[+] [-] disintegrator|4 years ago|reply
[+] [-] Joeri|4 years ago|reply
Don’t say what you were doing, say what the commit does. A good commit message completes the sentence “when merged this will …”.
Adding a more detailed description on the second line including why things were done is also not bad, but I have a strong opinion that design notes do not belong in the commit log but in code comments or markdown files that are in the repo, so that they can be found more easily and updated.
[+] [-] emptybottle|4 years ago|reply
The subject describes what is being affected (think easily greppable keywords)
Then, then body should go into detail about the contents and rationale for the changes.
Use specific terms like “add” or “remove” and avoid subjective descriptions like “prepare” “clean” and “fix”
For example this patch says:
> Clean up consistency of tabs/spaces in include_directories.
Well, what does clean up tabs/spaces actually mean? Saying “remove tabs in favor of spaces” is much clearer.
[+] [-] fnord77|4 years ago|reply
[+] [-] i386|4 years ago|reply
[+] [-] ricardobeat|4 years ago|reply
[+] [-] syspec|4 years ago|reply
https://user-images.githubusercontent.com/65677710/151421432...
--
(This and 2 other videos are linked in PR conversation tab: https://github.com/obsproject/obs-studio/pull/5875)
[+] [-] DoctorOW|4 years ago|reply
[+] [-] yellow_lead|4 years ago|reply
[+] [-] onion2k|4 years ago|reply
[+] [-] iSnow|4 years ago|reply
"When Steve came back, one of his company-wide edicts was that the names of individuals must be removed from about boxes."
https://bitsplitting.org/2014/01/24/cant-take-that-away/
[+] [-] mr90210|4 years ago|reply
[+] [-] viraptor|4 years ago|reply
[+] [-] helsinkiandrew|4 years ago|reply
Well done to who ever it was.
[+] [-] DangerousPie|4 years ago|reply
[+] [-] SloopJon|4 years ago|reply
[+] [-] olliej|4 years ago|reply
It could also be that someone would rather not be out in public, which is also understandable.
[+] [-] caaqil|4 years ago|reply
[+] [-] ripe|4 years ago|reply
It's open-source software for video recording and live streaming.
https://obsproject.com/
[+] [-] ru552|4 years ago|reply
[+] [-] oldiphone|4 years ago|reply
So they are mindful of people with old Macs. This is good!
[+] [-] artonge|4 years ago|reply
https://github.com/Developer-Ecosystem-Engineering?tab=overv...
https://github.com/obsproject/obs-browser/pull/310
Strangely, a lot of contributions to numpy.
[+] [-] ink404|4 years ago|reply
[+] [-] Oddskar|4 years ago|reply
I've internalized that there's a difference between the PR description, commit messages, and code comments. For me there's different intended audiences for all three, and their contents should thus be shaped accordingly. This PR basically combines all three into one and uses that as the commit message, and I'm not sure of what to make of it. Is this good practice?
Personally I find the contents of the "#### Summary of Changes" list to be super-neat and helpful. But should these not be more helpful as comments inside the code? On the other hand, it does explain what the code is doing at this snapshot in time: which makes it more precise than comments in the code -- since future changes might eventually make those comments incorrect.
What's your take?
[+] [-] franga2000|4 years ago|reply
If changes are made to the code, the comments should be updated accordingly. Getting from code to a PR text is a non-obvious and difficult process and when parts of the code are updated, the trail is easily lost.
[+] [-] edgyquant|4 years ago|reply
[+] [-] whazor|4 years ago|reply
[+] [-] evanb|4 years ago|reply
[+] [-] Telemakhos|4 years ago|reply
[+] [-] veenified|4 years ago|reply
[+] [-] underdeserver|4 years ago|reply
Seems like a money YouTuber, I've never watched him myself.
[+] [-] avipars|4 years ago|reply
[+] [-] clone1018|4 years ago|reply
The branch on GitHub should change its reference until the build system is merged into the main branch.
[+] [-] jagger27|4 years ago|reply
0: https://github.com/carlosonunez/obs-installer-for-apple-sili...
[+] [-] lelandfe|4 years ago|reply
Natively.
There's an open PR[0], #5155, to get OBS native on Apple Silicon. That's also what your weird fork uses.
Apple's PR is on top of that work: you'll see that there are 10 or so initial commits. Thus #5155 will need to be merged first, before Apple's.
#5155 is a huge PR and has been open for 5 months. Its release will also necessitate OBS get an M1 testing solution figured out. Here's a quote[2] from an OBS team member two days ago:
> [This PR is] top of our priority list once we release 27.2. We're keenly aware of how badly people want a native Apple Silicon build, and I can assure you that we will definitely be publishing official test builds of OBS with M1 support as soon as we are able.
I would nevertheless expect for all of this to take quite a while.
[0] https://github.com/obsproject/obs-studio/pull/5155
[1] https://github.com/obsproject/obs-studio/pull/5155#issuecomm...
[+] [-] perryizgr8|4 years ago|reply
[+] [-] jscipione|4 years ago|reply
[+] [-] shantnutiwari|4 years ago|reply
Some time later, I tried OBS on Windows, and it just worked, first time, no fiddling required. And I realised the problem was on the Mac side, not Obs.
I know, its just one anecdote.
[+] [-] unknown|4 years ago|reply
[deleted]
[+] [-] programmarchy|4 years ago|reply
[+] [-] robbrown451|4 years ago|reply
[+] [-] stoicjumbotron|4 years ago|reply