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

Naive implementation in plugin system #40

Open
sillydan1 opened this issue Dec 20, 2022 · 9 comments
Open

Naive implementation in plugin system #40

sillydan1 opened this issue Dec 20, 2022 · 9 comments
Labels
good first issue 🧑‍💻 Good for newcomers help wanted 🙏 Some assistance is required

Comments

@sillydan1
Copy link
Owner

sillydan1 commented Dec 20, 2022

The function bool is_dynamic_library(const std::string& filename) implementation is quite naïve in the sense that it just checks if the filename contains dylib/so/dll.

This means that a file named foo.so.txt is considered a dynamic library and is the program attempts to load it as a plug-in.

A real solution would be to use terminfo to inspect what the given file is.

@sillydan1 sillydan1 added help wanted 🙏 Some assistance is required good first issue 🧑‍💻 Good for newcomers labels Dec 20, 2022
@kiner-shah
Copy link

Terminfo? Wouldn't that give information about terminal?

@sillydan1
Copy link
Owner Author

Terminfo? Wouldn't that give information about terminal?

Correct, I think I meant the 'file' command, or maybe https://en.cppreference.com/w/cpp/filesystem/file_type or similar

@kiner-shah
Copy link

Which file is this function in?

@sillydan1
Copy link
Owner Author

Which file is this function in?

Right, sorry. I was referring to the Linux file command 😄 I'm not sure if it is available on Windows though - and I'm also not sure which library it is part of 🤔

@kiner-shah
Copy link

I meant, which file is bool is_dynamic_library(const std::string& filename) in?

@sillydan1
Copy link
Owner Author

It's in the plugin_system.cpp 😄

@kiner-shah
Copy link

Maybe the check for is_dynamic_library isn't required. If dlopen fails, then it fails; instead of throwing a logic error, better to log a message "couldn't load file as shared/dynamic lib" and continue.

@sillydan1
Copy link
Owner Author

So, just remove

if (is_dynamic_library(entry.path().filename())) {
? Yea, that probably works just fine 🤔

I might also downgrade the warning log to an info level, just so you dont get weird warnings about being unable to load non-plugin files.

sillydan1 added a commit that referenced this issue Jul 2, 2024
@kiner-shah
Copy link

You may keep it at debug level actually, if a plugin is loaded, then a info level log would make sense.

sillydan1 added a commit that referenced this issue Jul 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue 🧑‍💻 Good for newcomers help wanted 🙏 Some assistance is required
Projects
None yet
Development

No branches or pull requests

2 participants