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

Fix parsing problems with FL Studio 20.9 project files. #98

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

Conversation

LeStahL
Copy link
Contributor

@LeStahL LeStahL commented Jan 26, 2022

Removed the Monad FL Parser binary from the repository. Added opt-in auto-download and default path override options for the FL parser to CMakeLists.txt. Changed referenced Monad FL parser version to be able to parse FL 20.9 project files.

Implements #97.

…auto-download and default path override options for the FL parser to CMakeLists.txt. Changed referenced Monad FL parser version to be able to parse FL 20.9 project files.
Copy link
Member

@yupferris yupferris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience while I wrapped up some other stuff :)

.appveyor.yml Outdated
@@ -17,12 +17,15 @@ install:
Invoke-WebRequest "https://web.archive.org/web/20200502121517/https://www.steinberg.net/sdk_downloads/vstsdk366_27_06_2016_build_61.zip" -OutFile "vstsdk.zip"
Expand-Archive "vstsdk.zip" "C:\tmp"
}
if (-Not (Test-Path "C:\tmp\Monad.FLParser.dll")) {
Invoke-WebRequest "https://github.com/LeStahL/FLParser/releases/download/compatibility-20.9/Monad.FLParser.dll" -OutFile "C:\tmp\Monad.FLParser.dll"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we maybe include the FL parser as source in a subdir, similar to how we handle the VST SDK, instead of linking an external binary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem! 'Other stuff' turned out great! :)

We could add the FL parser source at configure time and compile it, instead of downloading the binary, with only the implication of requiring a higher .NET build tools version than we do now. We have the options

(1) add the FL parser repo as git submodule.
(2) use CMake to download the source from the FL parser repo at configure time.

To build it together with WS, we could

(a) add a library target for the FL parser to the WaveSabreConvert CMakeLists.txt,
(b) modify the FL parser to include a CMakeLists.txt

Let me know which one you'd prefer. I think (2a) is the option most similiar to the VST SDK handling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem! 'Other stuff' turned out great! :)

cheers!

I like both 2a and 2b. Obviously 2a is more localized/practical and doesn't rely on external maintainers to do or accept changes in their repo. I especially like it if we can reference a specific commit, which means explicitly updating it from time to time, but I'd rather do that than have the rug pulled under us if/when breaking changes happen.

So yeah, tl;dr I like 2a, and perhaps we can look into 2b at some point in the future of monad wants to support cmake explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, changed this. Some other changes were necessary:

  • Removed the VS 2013 configuration, because it lacks the required C# language version for the FL parser.
  • Added the VS 2022 configuration, because it exists now and is interesting.
  • Changed the Wayback request URI (this seems to have changed in the meantime, but because of the cache, none of the other pipelines have failed so far).

If Monad decide, at some point, to unarchive their FL parser repo, we can use the now included CMakeLists.txt for a CMake-support-PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, and now it references a specific commit as requested :D

@LeStahL LeStahL force-pushed the fix-flp-null-reference branch from 045702b to 402a53b Compare February 5, 2022 15:07
@LeStahL LeStahL force-pushed the fix-flp-null-reference branch from 402a53b to a7566c2 Compare February 5, 2022 15:22
@LeStahL LeStahL force-pushed the fix-flp-null-reference branch from b3d5bd2 to 7a9ddc5 Compare February 5, 2022 16:14
@LeStahL
Copy link
Contributor Author

LeStahL commented Nov 15, 2022

After using the code in this PR for a while, I have mixed feelings about this PR: I should remove the externalproject mechanism. It's unreliable.
I'd prefer a FLParser git submodule containing the CMakeLists.txt and the 20.9 FLP fix as commit. Maybe as logicomacorp fork of the original FLParser (it's unmaintained atm)?
The FLParser dll should not be in the tree imo (it's GPL).
Also, this PR should be several PRs, as it's addressing multiple things at once.

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

Successfully merging this pull request may close these issues.

2 participants