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

Remove the need of set up environment variables in CLI or MetaCall #233

Closed
viferga opened this issue Jan 19, 2022 · 4 comments · Fixed by #244
Closed

Remove the need of set up environment variables in CLI or MetaCall #233

viferga opened this issue Jan 19, 2022 · 4 comments · Fixed by #244
Assignees
Labels
c/c++ Pull requests that update C/C++ code enhancement New feature or request good first issue Good for newcomers

Comments

@viferga
Copy link
Member

viferga commented Jan 19, 2022

🚀 Feature

MetaCall has the need to set up multiple environment variables ( https://github.com/metacall/core/blob/develop/docs/README.md#42-environment-variables ), for example, in order to find the global configuration or plugins. This can be basically avoided if we use a trick for locating libmetacall.so / metacall.dll / libmetacall.dylib from the executable itself. This solution is platform dependent but we can use it just as a fallback in case the environment variables are not detected, so MetaCall is still able to work as normally it does in multiple platforms.

The trick is the following:

  1. First of all, we get the list of the modules that the current executable (usually metacallcli) is linked too, this includes dynamic loaded modules, as in the case (in the future) we will support MetaCall to be imported from python.exe or node.exe ( Allow MetaCall to run from python.exe or node.exe #231 ), and in this case libmetacall.so / metacall.dll / libmetacall.dylib will be linked dynamically by dlopen or LoadLibrary.
  2. Once we get the list, we try to find libmetacall.so / metacall.dll / libmetacall.dylib (taking into account versioning in the future if any, probably we need some way of parsing the library name or finding a symbol inside of the library in order to verify that it is the correct library).
  3. We extract the absolute path of this module, and then we can obtain all the modules from there. For example, let's say we find MetaCall library in: /home/custom/metacall/lib/libmetacall.so, we can take this path and obtain all the environment variables from it. All the *_LIBRARY_PATH would be pointing to /home/custom/metacall/lib and the configuration CONFIGURATION_PATH can be deduced from it too and it will be pointing to /home/custom/metacall/configurations/global.json.

With this we remove all the need for setting up metacall and we improve the user experience with it, specially when using Ports because it is problematic to set them up properly.

Something we must review and it's critical in order to make this work properly, some loaders rely on getting the environment variables by themselves, specially in node loader or typescript loader with bootstrap.js and bootstrap.ts. We should remove the environment variable access from them and pass them from the C/C++ side, so it works too. This can be done easily because in the initialize step of each loader, we pass the configuration with the required paths as values, we can take them from there and pass them into JavaScript land.

Some of the reference for implementing this can be found here:

The function for finding the module path can be implemented either on dynlink or in portability (dynlink can be interesting because it contains suffixes and prefixes for the dynamic libraries for each platform).

@viferga viferga added good first issue Good for newcomers c/c++ Pull requests that update C/C++ code enhancement New feature or request labels Jan 19, 2022
@burnerlee
Copy link
Contributor

@viferga I would like to get started by picking up this issue. Can you please assign it to me?

@viferga
Copy link
Member Author

viferga commented Jan 24, 2022

The order of priority for setting up the paths is:

  1. User defined environment variables.
  2. Paths obtained from the mechanism defined before.
  3. Compile time default paths.

The paths (and environment variables) are initialized here:

void loader_env_initialize(void)

if (singleton->library_path == NULL)

static const char configuration_path[] = CONFIGURATION_PATH;

if (singleton->library_path == NULL)

As you can see the code is pretty copy pasted. It can be improved but there's no need to do so in this patch, you can focus only in removing the need of setting up the environment variables.

My suggestion is to create a function that obtains the prefix path when metacall initializes (in metacall_initialize or similar), and then this can be queried by the different modules in order to obtain the respective path and then generate its own paths.

Just a reminder, we snake case (function_hello) instead of camel case (functionHello).

@burnerlee
Copy link
Contributor

@viferga I wanted to discuss this approach with you. I was planning to define a function set_env_path_from_metacall_lib which does the following job:

  • finds the custom path e.g. /usr/local using the location of shared library. Let's call this metacall_custom_path.
  • then for each env variable, it would check if the env variable has been setup by the user. If any env is NULL or an empty string, it would calculate it's value using the metacall_custom_path and then set this as the value of the env variable.
    Then we can call this function as soon as metacallcli is initialised before any env variables are accessed. Then, effectively we would use default values wherever env variables were not initialised after the 2 checks. This would help reduce copy pasting the code at multiple places where env variables are accessed.
    I'm not sure if this method have some loophole, or is not the right way to do.

@viferga
Copy link
Member Author

viferga commented Jan 25, 2022

I think it's fine but I would start doing the function that gives you the path, and only that, then create a unit test for it.

Once you have this, which works independently of metacall, then you can try to integrate this into the codebase. With unit tests you ensure your function works properly. I can help you to set up them. We use a lot Test Driven Development in this project, it helps us a lot to verify that the changes we do work properly.

Also, depending on the module we are, usually we set the module name as a prefix. For example metacall module has functions starting by metacall_*, the loader module has functions starting by loader_*, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/c++ Pull requests that update C/C++ code enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants