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

feat: add basic add_commonlibsf_plugin command #31

Closed
wants to merge 3 commits into from

Conversation

ThirdEyeSqueegee
Copy link
Member

Not sure if linking fmt explicitly is required since everything worked fine without doing that but went ahead and did it anyway

@gottyduke
Copy link
Contributor

gottyduke commented Sep 11, 2023

Not sure if linking fmt explicitly is required since everything worked fine without doing that but went ahead and did it anyway

Then why doing it? spdlog already has fmt as a dependency, statement is redundant.

Again, I would like to ask you to actually test the code first, this addition has only swapped the add_library call in plugin's CMakeLists.txt with add_commonlibsf_plugin, it has no other integrations in CLib itself. Maybe actually implement the rest of the cmake commands that parses and generates Plugin.h?


Since you are adding the cmake command first instead of code, assumes the proper plugin declaration code has been implemented within CLib :

set(options OPTIONAL USE_SIGNATURE_SCANNING)

The options need to extend to all version independence properties.

And this will render #24 getters useless. If the plugins will be using all wrapped up CLib plugin declarations and macros, then user should not be able to view the raw or modify SFSEPlugin_Version directly.

@ThirdEyeSqueegee
Copy link
Member Author

Seems like full integration the way CLibNG does would necessitate getting rid of the submodule approach for consuming the library

@gottyduke
Copy link
Contributor

Seems like full integration the way CLibNG does would necessitate getting rid of the submodule approach for consuming the library

How did you come up with this conclusion? CLibNG could be consumed via submodule just as easily as the vcpkg port approach. Are you trying to replace the current PluginAPI with PluginDeclaration implementation again like this commit?

@ThirdEyeSqueegee
Copy link
Member Author

No that's not what I'm trying to do, I'm just having some trouble testing my changes--just updated the PR, maybe someone can validate

@ThirdEyeSqueegee ThirdEyeSqueegee changed the title feat: add basic add_commonlibsf_plugin command, add header-only fmt feat: add basic add_commonlibsf_plugin command Sep 11, 2023
@ThirdEyeSqueegee ThirdEyeSqueegee marked this pull request as draft September 11, 2023 03:41
CommonLibSF/include/SFSE/Interfaces.h Outdated Show resolved Hide resolved
CommonLibSF/include/SFSE/Interfaces.h Show resolved Hide resolved
@ThirdEyeSqueegee ThirdEyeSqueegee linked an issue Sep 11, 2023 that may be closed by this pull request
@ThirdEyeSqueegee ThirdEyeSqueegee deleted the feature/cmake_command branch September 14, 2023 22:41
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.

add_commonlibsf_plugin CMake command
2 participants