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

[Windows] Add multi os wrapper for loading dynamic libraries #2879

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

gkisalapl
Copy link
Contributor

Create class which make possible to use the same interface for loading dynamic libraries and symbols for multiple operating systems

Self-evaluation:

  1. Build test: [X]Passed [ ]Failed [ ]Skipped
  2. Run test: [X]Passed [ ]Failed [ ]Skipped

Copy link
Member

@DonghakPark DonghakPark left a comment

Choose a reason for hiding this comment

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

  1. Please Fill Commit body message on below commits for log & clear understanding of others
  1. Ignore doxygen tag CI Failed, for external file we don't need to make doxygen tags

@myungjoo
Copy link
Member

@DonghakPark We are exporting every header files to .dev package. But, this is not a good practice and we need to choose header files that are needed by external packages and exclude other headers from .dev package. Opening up all the symbols to external packages is not a good design choice (you are declaring all functions as APIs by doing so.)

@DonghakPark
Copy link
Member

@DonghakPark We are exporting every header files to .dev package. But, this is not a good practice and we need to choose header files that are needed by external packages and exclude other headers from .dev package. Opening up all the symbols to external packages is not a good design choice (you are declaring all functions as APIs by doing so.)

That's correct.
Recently, during development, we haven't had enough time to sort out these aspects. we'll discuss this matter with other team members so that we can distinguish APIs (open or not) that will be released at the completion of FSU, SubGraph, and Op(ONNX) developments

@myungjoo
Copy link
Member

myungjoo commented Jan 18, 2025

@DonghakPark We are exporting every header files to .dev package. But, this is not a good practice and we need to choose header files that are needed by external packages and exclude other headers from .dev package. Opening up all the symbols to external packages is not a good design choice (you are declaring all functions as APIs by doing so.)

That's correct. Recently, during development, we haven't had enough time to sort out these aspects. we'll discuss this matter with other team members so that we can distinguish APIs (open or not) that will be released at the completion of FSU, SubGraph, and Op(ONNX) developments

It's Saturday morning. Go somewhere else!

Anyway, I recommend to put externally available headers into a designated directory for easier maintanance.
Plus, we may make a rule that refers to externally available headers with #include <ext-header.h> and internally available headers with #include "dir/int-header.h".

Copy link
Contributor

@djeong20 djeong20 left a comment

Choose a reason for hiding this comment

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

Nice work!

#define RTLD_DEEPBIND 0
#define RTLD_GLOBAL 0
#define RTLD_LOCAL 0
#define RTLD_NODELETE 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of the flags, such as RTLD_BINDING_MASK, are currently unused. Will we use these unused flags in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added all flags defined in bits/dlfcn.h linux header to ensure full compatibility.
I do not think that we will use all of them.

Copy link
Member

@myungjoo myungjoo Jan 22, 2025

Choose a reason for hiding this comment

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

Or.. we may hide them by:

class DynamicLibraryLoader {
public:
  static void *loadLibrary(const char *path, [[maybe_unused]] int lazy = 0) {
#if defined(_WIN32)
    return LoadLibraryA(path);
#else
    return dlopen(path, (lazy ? RTLD_LAZY : RTLD_NOW) | RTLD_LOCAL);
#endif
  }

@gkisalapl gkisalapl force-pushed the dynamic_library_loader branch from 8a8b97a to 595559c Compare January 20, 2025 14:21

Verified

This commit was signed with the committer’s verified signature.
paescuj Pascal Jufer
Create class which make possible to use the same interface for loading dynamic libraries and symbols for mutliple operating systems

**Self-evaluation:**
1. Build test: [X]Passed [ ]Failed [ ]Skipped
2. Run test:   [X]Passed [ ]Failed [ ]Skipped

Signed-off-by: Grzegorz Kisala <[email protected]>
@gkisalapl gkisalapl force-pushed the dynamic_library_loader branch from 595559c to 49037e7 Compare January 20, 2025 14:21
Copy link
Collaborator

@jijoongmoon jijoongmoon left a comment

Choose a reason for hiding this comment

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

LGTM!

@jijoongmoon jijoongmoon merged commit b18583f into nnstreamer:main Jan 23, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants