-
Notifications
You must be signed in to change notification settings - Fork 148
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
Update plugin template repository with code changes from obs-studio repository #126
base: master
Are you sure you want to change the base?
Conversation
ad178d3
to
83c8d4f
Compare
Requires obsproject/obs-studio#11114 to be merged and the OBS tag/release to be updated for the plugintemplate. |
The obs-studio PR has been merged, but there is no new tag/release yet. Would it be possible to get an intermediate update for 30.2.x in the mean time? |
For the plugintemplate you mean? |
Yes. |
I'd have to check, wholly depends on whether the legacy path on Windows generates the libraries the way the update needs them to, can check that later. |
I mostly just mean would it make sense to update buildspec for 30.2.x in the meantime, or are there other changes required? |
In that case I'd let a different PR take care of that, because this PR is much larger in scope. |
Yes, that's what I was suggesting. |
I was going to create a PR to update the codesign action to obsproject/obs-studio#11270 but now saw this one, could it be updated to include the changes from the lastest OBS master branch (specifically the |
That would be the plan. I haven't updated the PR yet because it sounds like it would not be merged before OBS Studio's next major release anyway. |
Question, why did you remove all switches from Build/Package-Windows.ps1? I see you wrote something about making these CI only. What was the reasoning for it? Also I added a PR #128 fixing Inno Setup, since you already have it. Might be worth integrating into this one. Although it might be incomplete in terms of the $Prefix paths. |
The build scripts are only intended for use on CI, which is why they were placed in Furthermore this PR will be updated soon and remove the Maintaining these scripts for purposes other than that is unfeasible for the project, as it has led to feature creep in the past (as developers understandably wanted their own particular needs and features become supported by the build/packaging scripts) and just introduced more potential for errors. Developers are of course free to maintain their own scripts in the The main entry point for configuring/building/installing are the CMake scripts, thus the shell scripts have become little more than "CMake command line generators". The "build" scripts in particular are a bit superfluous, it's mainly the "package" scripts that have not been migrated to CMake yet, though it would make sense to move that functionality entirely into the Ideally, everything should be achievable by running |
On Thu, Oct 10, 2024 at 5:39 PM Patrick Heyer ***@***.***> wrote:
Question, why did you remove all switches from Build/Package-Windows.ps1?
I see you wrote something about making these CI only. What was the
reasoning for it?
Also I added a PR #128
<#128> fixing Inno
Setup, since you already have it. Might be worth integrating into this one.
Although it might be incomplete in terms of the $Prefix paths.
The build scripts are only intended for use on CI, which is why they were
placed in .github from the get-go and not in build-aux. We also removed
any suggestion for their use in the README for the same reason.
Furthermore this PR will be updated soon and remove the .Wingetfile and
Install-BuildDependencies script entirely as they are neither needed for
CI (both CMake and InnoSetup are preinstalled on GitHub runners) nor were
they ever functional (winget is not available on GitHub Actions runners).
Maintaining these scripts for purposes other than that is unfeasible for
the project, as it has led to feature creep in the past (as developers
understandably wanted their own particular needs and features become
supported by the build/packaging scripts) and just introduced more
potential for errors.
I do understand the motivation to not complicate the CI scripts. However,
it does cause difficulties for those of us only lightly connected to the
main project. I've depended quite heavily on being able to exactly
reproduce one the CI build on my local machine. When things go wrong I can
debug it, and I've got much higher confidence that what is building on my
local machine is going to be successful when it gets run by CI.
Needing to keep up with changes to the build system when in-tree scripts
aren't suitable I've found to be quite demotivating. :-(
… Message ID: ***@***.***
com>
|
As I mentioned in my comment, by the point this PR is merged there is not much difference between what's run on CI and what you can run locally as the build scripts don't do a whole lot more than compose a CMake invocation like Lots of functionality the build scripts were necessary for (e.g., downloading and setting up the pre-built dependencies) has been moved into the CMake scripts, so the build scripts' job has literally been reduced to setting up some CI related stuff and invoking CMake's configure, build, and install steps in that order. The other old functionality was to automatically install build dependencies such as CMake itself, but that created more issues than it solved because we could not possibly support all local environments and instead ran the risk of unnecessarily fussing up those environments (e.g. by installing CMake a second time because we couldn't find it via the script). It's reasonable to expect developers to read the README and install the necessary build dependencies themselves (which puts the onus on us to properly document them there) rather than having automatic scripts interfere with local machine state(s). So by the point this PR is merged, all you should need to do is run I also think it's reasonable to expect plugin developers to understand what CI does, how builds are created, and how their plugins work internally as OBS Studio's module system gives you unlimited access to the internal functionality of it. With great power comes great responsibility. My personal goal is to have as much stuff handled by CMake as that makes it easier to generate cross-platform functionality (no need to juggle Zsh, Bash, and PowerShell) and would require developers at most to know the correct CMake invocations. |
6317fad
to
1ab1897
Compare
Went through the changes. The README is very thorough for beginners, especially people not well versed with CMake and I see you kept the Inno Setup script generation and added the documentation for it on the README. Also question, what is going on with clang format? I see a commit that you've replaced it with another solution, but you've also changed the .clang-format. Are you using the new one only on CMake files, but still expect clang-format to be used on the plugin files?! Can you explain this a bit? My only hesitation is setting the obs-studio version to a beta. Shouldn't it be a stable release since this is meant for plugin devs who'll be having to debug their own code? |
The beta update is just a temporary measure to get CI to pass - we recently updated As for |
So gersemi is not relevant to the user, just the maintainers? Not entirely sure how it plays into this, hence the question. We'll still need to use to clang-format tool right? Yeah, the SDK was old and cannot even be installed anymore. |
Gersemi is a CMake formatter, that is unrelated to The old By itself the changes should not create immediate issues, it is only when new source files are added or existing ones added that they need to be formatted using the new rules or the new formatter (as CI will only complain about incorrectly formatted files when they are part of a change). |
Updated debugging instructions require obsproject/obs-studio#11468 to be merged to be valid on macOS. |
We cannot (and don't want) to assume which configuration is to be used here, this needs to be an explicit choice by the developer, but it makes sense to add it explicitly to the documentation of the
EDIT: Discussed this internally - the least problematic location is in
I'm more and more inclined to just straight-up remove the InnoSetup files entirely. It just adds complexity and maintenance effort to the template and the choice of setup program (MSI, InnoSetup, etc.) should be up to the developer. As you correctly pointed out the current InnoSetup template file expects a very specific layout of files (which is only generated correctly when Trying to make the template file dynamically adjust to that is not worth the effort and would just open up even more possible ways it could break. So my bottom line on this is that if we provide convenience, it should be actually convenient and not "convenient just if you follow these very specific instructions and only if you do these specific things, and outside of that it won't work or break in opaque ways". |
7f2df24
to
e288c6a
Compare
My points were mostly about improving documentation(bumping up optional parameters to mandatory) so that even new users regardless of skill, can clone and build based on documentation and it works. I don't really mind being told how to do it, if I can have something that builds and can start doing what I need to, write the plugin. As it is, the changes I recommended were given after going through the latest README changes and it failed on me a few times. And even though I personally had the luxury to learn CMake(and dig into the cmake files you've set up) and work things out, I know from experience, that I could have easily landed here for something work related and it would have been demoralizing to waste time battling the build system instead of proceeding with the actual development. As for the Inno Setup, I get your frustration. My solution was mostly to allow more configurability without having to make it "smart". You're just exposing an extra parameter for the user to be able to set the very same directory that was the reason it failed, on their own. Having dealt with installers before, I know it's a chore in certain aspects, so you providing this out of the box is a huge convenience on its own, so personally I wouldn't like to see it go, but I understand your points as a maintainer. That said, after one of your older replies, I opted to modify Package-Windows into another script and exposed its functionality as Commands in vscode through a custom extension. Tasks are annoying to call, having that extra step, all the time. So I won't be affected without /D. Anyway, thanks for taking the time to respond. |
The default install location has already been updated yesterday, as for the setup stuff I'll give it a good night's sleep, but I gravitate toward removing it from the default build system and instead move the code into a wiki page "Example: How to use InnoSetup to build your plugin on CI". |
I'm not sure whether the resulting error is caused by this PR specifically (there are some changes related to the Ubuntu version), but in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nits. Looks fine otherwise, and is waiting on the pending stable release of OBS Studio 31.
8c9fd59
to
7e8817b
Compare
Extensive documentation has been added to the Wiki, superseding any information that was contained in the README (which itself has been slimmed down accordingly). Automatic generation of an installer for Windows has been removed for reasons outlined above (brittle setup, high maintenance burden). The wiki and updated code now default to generating a plugin that needs to be installed to "ProgramData" and removes compatibility to run such plugins from OBS Studio's program directory. |
I tested this new plugin template on Ubuntu 24.04, latest OBS 31.0.0 installed through ppa+apt install. I ran the following: cmake --preset linux-x86_64
cmake --build --preset linux-x86_64
sudo cmake --install build_x86_64 but it installed the plugin in cmake --preset linux-x86_64 -DCMAKE_INSTALL_PREFIX=/usr -DCMAKE_INSTALL_LIBDIR=lib/x86_64-linux-gnu and it worked. Have the current default locations been tested on Ubuntu before? |
I mentioned this in an earlier post. Setting CMAKE_INSTALL_PREFIX needs to be mandatory in the README for this to work properly. By default, CMake uses weird places. On linux, it defaults to /usr/local/ and the install command has a destination of |
Setting Which is why the Wiki highly recommends reading the CMake Tutorial in the first place, as we cannot (and should not) explain main CMake functionality if there are already good resources for just that out there. The prefix part is also mentioned on this Wiki page explaining the separate CMake build system steps. |
My reply was directly on how @loicmagne managed to correct the install command and I omitted that "it's mandatory to override it using --prefix on the install command" for it to work properly. Last I remember the README was the one containing the instructions, but I now see you rolled some more changes that now point to the wiki. Looking at the wiki, looks great. Nice amount of info. |
Yeah it was just too much stuff to roll into a README, the Wiki is the better place for it and allowed for much more detailed information and links to further information. The problem with the prefix is that there is no good "default" for Linux, as fragmented as the ecosystem is, so CMake went with the POSIX default (and these days macOS is POSIX-compliant but Linux is not, so YMMV). |
The issue I have with When I look at a working plugins, the install locations are: $ find /usr -name "input-overlay"
/usr/share/obs/obs-plugins/input-overlay
$ find /usr -name "input-overlay.so"
/usr/lib/x86_64-linux-gnu/obs-plugins/input-overlay.so However if I use the $ find /usr -name "obs-test"
/usr/share/obs/obs-plugins/obs-test
$ find /usr -name "obs-test.so"
/usr/lib/obs-plugins/obs-test.so The I have little knowledge about Linux install locations and CMake so there might be something I'm missing |
How do you configure your project? You should use presets (as we have documented in the Wiki now), which automatically add the If your distribution requires a different variant, you either need to add custom presets or indeed overwrite via |
7e8817b
to
a7c91a2
Compare
a7c91a2
to
4b241c8
Compare
Description
Updates CI, CMake, and build scripts to port over changes made by/for the
obs-studio
repo.Changes include:
cmake-format
togersemi
as code-formatting tool for CMake filesobs-studio
)aarch64
toolchains and preset support for Linux (due to lack ofobs-studio
compatibility)obs-studio
)Fixes #123.
Fixes #124.
Fixes #116.
Motivation and Context
Plugin template repository should be regularly updated to stay in sync with developments that occurred on
obs-studio
.How Has This Been Tested?
Local builds successfully tested on macOS 14, CI changes require CI runs to test.
Types of changes
Checklist: