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

Update plugin template repository with code changes from obs-studio repository #126

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

PatTheMav
Copy link
Member

Description

Updates CI, CMake, and build scripts to port over changes made by/for the obs-studio repo.

Changes include:

  • Switch from cmake-format to gersemi as code-formatting tool for CMake files
  • Split up macOS and Linux build scripts to allow more independent adaptation to Linux requirements
  • Made macOS build scripts CI only (just like on obs-studio)
  • Removed aarch64 toolchains and preset support for Linux (due to lack of obs-studio compatibility)
  • Made Windows build scripts CI only (just like on obs-studio)
  • Updated dependency hashes

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

  • Code cleanup (non-breaking change which makes code smaller or more readable)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@PatTheMav
Copy link
Member Author

Requires obsproject/obs-studio#11114 to be merged and the OBS tag/release to be updated for the plugintemplate.

@RytoEX
Copy link
Member

RytoEX commented Aug 20, 2024

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?

@PatTheMav
Copy link
Member Author

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?

@RytoEX
Copy link
Member

RytoEX commented Aug 20, 2024

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.

@PatTheMav
Copy link
Member Author

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.

@RytoEX
Copy link
Member

RytoEX commented Aug 20, 2024

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?

@PatTheMav
Copy link
Member Author

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.

@RytoEX
Copy link
Member

RytoEX commented Aug 21, 2024

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.

@gxalpha
Copy link
Member

gxalpha commented Sep 27, 2024

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 setup-macos-codesign action) before merging?

@PatTheMav
Copy link
Member Author

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 setup-macos-codesign action) before merging?

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.

@NPatch
Copy link

NPatch commented Oct 10, 2024

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.

@PatTheMav
Copy link
Member Author

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 .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.

Developers are of course free to maintain their own scripts in the build-aux subdirectory, which should be somewhat safe from merge conflicts, even though I'd still advise developers to chart their own path from the moment they create a new repository with the template and don't stay tethered to it (e.g., not every change made to the template needs to be implemented by existing plugins).

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 cmake --package stage.

Ideally, everything should be achievable by running cmake and not invoke any external scripts. We've been oscillating between doing stuff in shell scripts vs. having CMake take care of it as our requirements and CMake's capabilities have evolved over time.

@glikely
Copy link
Contributor

glikely commented Oct 11, 2024 via email

@PatTheMav
Copy link
Member Author

PatTheMav commented Oct 11, 2024

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. :-(

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 cmake --preset windows-ci-x64.

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 cmake --preset <your-platform-preset>, then open the IDE of your choice and build the project in whichever config you need. CI will use Release config for releases and RelWithDebInfo otherwise. There is no additional "secret sauce" added by the build scripts that you need to worry about.

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.

@PatTheMav PatTheMav force-pushed the repo-update branch 4 times, most recently from 6317fad to 1ab1897 Compare October 11, 2024 15:53
@NPatch
Copy link

NPatch commented Oct 11, 2024

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?

@PatTheMav
Copy link
Member Author

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 obs-studio to accommodate CMake changes with regards to the Windows SDK version used with the Visual Studio generators which are also included in this update and it would break otherwise.

As for clang-format, it just updated the column limit to 120 characters.

@NPatch
Copy link

NPatch commented Oct 11, 2024

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.

@PatTheMav
Copy link
Member Author

So gersemi is not relevant to the user, just the maintainers? Not entirely sure how it plays into this, hence the question.

Yeah, the SDK was old and cannot even be installed anymore.

Gersemi is a CMake formatter, that is unrelated to clang-format, which formats C/C++/ObjC source code.

The old cmake-format formatter hasn't been updated in years whereas gersemi is actively maintained and has been very reactive to our support requests.

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).

@PatTheMav
Copy link
Member Author

Updated debugging instructions require obsproject/obs-studio#11468 to be merged to be valid on macOS.

@PatTheMav
Copy link
Member Author

PatTheMav commented Oct 31, 2024

  • For the cmake install command, it needs the --config RelWithDebInfo, because cmake_install.cmake:39 expects the dll in the Release config otherwise.

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 cmake --install step and note that this should match the configuration used to build the project.

  • Also cmake install defaults to Program Files/plugin-name, if not overriden somehow, on Windows. So far, I've been using Build-Windows.ps1, which would copy all built files to the release folder regardless of CMAKE_INSTALL_PREFIX.

EDIT: Discussed this internally - the least problematic location is in %APPDATA% which is used by OBS Studio on Windows by default except if portable mode is enabled. So we'll override CMake's default to use that location if no prefix is provided.

Also if you override the path on cmake install with --prefix, then we also need to add the /D switch on the iscc call to override the Source directory the script will try to look into for the source files, because the installer-Windows.iss.in:28 looks for:

Source: "..\release\Package\*";

which unless we used --prefix to point to the release folder, it won't exist. The .iss.in was written to default to the same release/Package directory the Package-Windows.ps1 created prior to the call and destroyed afterwards.

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 cmake --install is run with a specific prefix) and will fall apart once that is changed.

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".

@PatTheMav PatTheMav force-pushed the repo-update branch 3 times, most recently from 7f2df24 to e288c6a Compare October 31, 2024 18:19
@NPatch
Copy link

NPatch commented Oct 31, 2024

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.

@PatTheMav
Copy link
Member Author

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".

README.md Outdated Show resolved Hide resolved
@gxalpha
Copy link
Member

gxalpha commented Nov 2, 2024

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 .github/workflows/push.yaml there is a mention of ubuntu-22.04-x86_64;tar.xz|deb|ddeb which also needs adjusting the version. Otherwise, the file rename/moving during the "Create Release" won't happen on the Ubuntu artifacts and they will not appear in the release as a result.

Copy link
Member

@RytoEX RytoEX left a 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.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@RytoEX RytoEX self-assigned this Nov 5, 2024
@PatTheMav PatTheMav force-pushed the repo-update branch 4 times, most recently from 8c9fd59 to 7e8817b Compare November 14, 2024 20:28
@PatTheMav
Copy link
Member Author

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.

@loicmagne
Copy link

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 /usr/local/lib/obs-plugins/ and it wouldn't be loaded by OBS. I changed the install prefix with:

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?

@NPatch
Copy link

NPatch commented Dec 16, 2024

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 /usr/local/lib/obs-plugins/ and it wouldn't be loaded by OBS. I changed the install prefix with:

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 CMAKE_INSTALL_LIBDIR/obs-plugins. This is how your /usr/local/lib/obs-plugins directory is defined.

@PatTheMav
Copy link
Member Author

PatTheMav commented Dec 16, 2024

Setting CMAKE_INSTALL_PREFIX is only necessary if cmake --install is not used, otherwise the preferred way is to specify the prefix directly to that command per the CMake Tutorial.

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.

@NPatch
Copy link

NPatch commented Dec 16, 2024

Setting CMAKE_INSTALL_PREFIX is only necessary if cmake --install is not used, otherwise the preferred way is to specify the prefix directly to that command per the CMake Tutorial.

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.

@PatTheMav
Copy link
Member Author

Setting CMAKE_INSTALL_PREFIX is only necessary if cmake --install is not used, otherwise the preferred way is to specify the prefix directly to that command per the CMake Tutorial.

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).

@loicmagne
Copy link

loicmagne commented Dec 16, 2024

Setting CMAKE_INSTALL_PREFIX is only necessary if cmake --install is not used, otherwise the preferred way is to specify the prefix directly to that command per the CMake Tutorial.

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.

The issue I have with --prefix is that it doesn't allow to specify different locations for .so library file and the data files

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 --prefix option like sudo cmake --install build_x86_64 --prefix /usr/, this is where the package is installed:

$ 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 .so file end up in an incorrect location. I couldn't find a --prefix value that installs both library/data to the correct location which is why I had to use -DCMAKE_INSTALL_PREFIX=/usr -DCMAKE_INSTALL_LIBDIR=lib/x86_64-linux-gnu

I have little knowledge about Linux install locations and CMake so there might be something I'm missing

@PatTheMav
Copy link
Member Author

Setting CMAKE_INSTALL_PREFIX is only necessary if cmake --install is not used, otherwise the preferred way is to specify the prefix directly to that command per the CMake Tutorial.
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.

The issue I have with --prefix is that it doesn't allow to specify different locations for .so library file and the data files

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 --prefix option like sudo cmake --install build_x86_64 --prefix /usr/, this is where the package is installed:

$ 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 .so file end up in an incorrect location. I couldn't find a --prefix value that installs both library/data to the correct location which is why I had to use -DCMAKE_INSTALL_PREFIX=/usr -DCMAKE_INSTALL_LIBDIR=lib/x86_64-linux-gnu

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 x86_64-linux-gnu part.

If your distribution requires a different variant, you either need to add custom presets or indeed overwrite via CMAKE_INSTALL_LIBDIR, but that's due to different distributions not agreeing on this location.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants