search

obsproject / obs-plugintemplate

GNU General Public License v2.0
307 stars 141 forks source link

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

Open PatTheMav opened 3 months ago

PatTheMav commented 3 months ago

Description

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

Changes include:

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:

PatTheMav commented 3 months ago

Requires https://github.com/obsproject/obs-studio/pull/11114 to be merged and the OBS tag/release to be updated for the plugintemplate.

RytoEX commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 1 month ago

I was going to create a PR to update the codesign action to https://github.com/obsproject/obs-studio/pull/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 commented 1 month ago

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 commented 1 month ago

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 https://github.com/obsproject/obs-plugintemplate/pull/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 commented 1 month ago

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 commented 1 month ago

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 https://github.com/obsproject/obs-plugintemplate/pull/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>

PatTheMav commented 1 month ago

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.

NPatch commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

NPatch commented 1 month ago

Thank you for taking the time to explain!

NPatch commented 1 month ago

I have the following propositions:

So the --prefix switch on cmake install is not optional if you want everything to work without fiddling around after checkout.

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.

An alternative would be to do the following in the .iss.in:

+ #ifndef SourceDir
+ #define SourceDir "..\release\Package"
+ #endif

[Files]
- Source: "..\release\Package\*"; DestDir: "{app}"; Flags: ignoreversion recursesubdirs 
+ Source: "{#SourceDir}\*"; DestDir: "{app}"; Flags: ignoreversion recursesubdirs

The ifndef/define defaults to what the powershell expects. But if we use the /DSourceDir="....", we can override it from command line. And it becomes imperative to add to the README.

I know that you said you want to pass the onus to the developer for some things, but I'm thinking about working state after checking out the project. The above changes would allow even a newbie to get sth that compiles and installs and everything out of the box.

PatTheMav commented 3 weeks ago

Updated debugging instructions require https://github.com/obsproject/obs-studio/pull/11468 to be merged to be valid on macOS.

PatTheMav commented 3 weeks ago
  • 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".

NPatch commented 3 weeks ago

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 commented 3 weeks ago

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

gxalpha commented 3 weeks ago

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.

PatTheMav commented 1 week ago

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.