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

Dynamic load libraries for external functions #28

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

Conversation

artP2
Copy link
Contributor

@artP2 artP2 commented Jan 14, 2025

Things that need attention:

  • Where the (standard) library directory will be located.
  • Write tests

@artP2 artP2 linked an issue Jan 14, 2025 that may be closed by this pull request
3 tasks
@artP2 artP2 force-pushed the feat/16/dynamic-libraries branch 2 times, most recently from 4bde6ee to cbb8fa2 Compare January 14, 2025 20:04
@artP2 artP2 force-pushed the feat/16/dynamic-libraries branch 4 times, most recently from 2a44623 to d4a5c83 Compare January 17, 2025 17:17
@artP2
Copy link
Contributor Author

artP2 commented Jan 28, 2025

Shouldn't library_api.h depend on the rest of the project? This would make sense for independent library developers.

@artP2 artP2 force-pushed the feat/16/dynamic-libraries branch 2 times, most recently from cc77d7e to 297e9d5 Compare February 3, 2025 13:05
@artP2 artP2 force-pushed the feat/16/dynamic-libraries branch from 297e9d5 to 039485d Compare February 11, 2025 17:23
@artP2 artP2 marked this pull request as ready for review February 11, 2025 17:28
@Grillo-0 Grillo-0 self-requested a review February 12, 2025 11:28
Copy link
Collaborator

@Grillo-0 Grillo-0 left a comment

Choose a reason for hiding this comment

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

Some first review comments.

The commit structure and message needs some work, it would be great if you describe more of each commit with a body, some of the time a single line doesn't do the trick.

@artP2 artP2 force-pushed the feat/16/dynamic-libraries branch from 1057782 to f5f95c1 Compare February 14, 2025 18:59
@artP2
Copy link
Contributor Author

artP2 commented Feb 14, 2025

My commits were quite messy, now it should be much more concise. But I'm not happy with library_api.h in the last commit, any suggestions?

@artP2 artP2 force-pushed the feat/16/dynamic-libraries branch from f5f95c1 to d19c9ec Compare February 14, 2025 19:23
@Grillo-0
Copy link
Collaborator

My commits were quite messy, now it should be much more concise. But I'm not happy with library_api.h in the last commit, any suggestions?

I don't see a problem with the library_api.h in the last commit, it is part of the stdlib and on this commit you are creating the stdlib.

Copy link
Collaborator

@Grillo-0 Grillo-0 left a comment

Choose a reason for hiding this comment

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

About the commit messages, much better, just one nitpick, capitalize the subject lines, and also you forgot to capitalize the body of the first commit

Comment on lines 217 to 239
if (symbol_node->type == FUNCTION){
size_t buffer_size = 1024;
char *buffer = (char*) calloc(1, buffer_size);
FILE *tmp_out = fmemopen(buffer, buffer_size, "w+");
for (size_t i = 0; i < args_node->childs.len; i++) {
do_calls(ast, tmp_out, VEC_AT(&args_node->childs, i));
}
fflush(tmp_out);
fprintf(out, "%s", symbol_node->fp(buffer));
fclose(tmp_out);
free(buffer);
}else{
for (size_t i = 0; i < args_node->childs.len; i++) {
do_calls(ast, out, VEC_AT(&args_node->childs, i));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (symbol_node->type == FUNCTION){
size_t buffer_size = 1024;
char *buffer = (char*) calloc(1, buffer_size);
FILE *tmp_out = fmemopen(buffer, buffer_size, "w+");
for (size_t i = 0; i < args_node->childs.len; i++) {
do_calls(ast, tmp_out, VEC_AT(&args_node->childs, i));
}
fflush(tmp_out);
fprintf(out, "%s", symbol_node->fp(buffer));
fclose(tmp_out);
free(buffer);
}else{
for (size_t i = 0; i < args_node->childs.len; i++) {
do_calls(ast, out, VEC_AT(&args_node->childs, i));
}
if (symbol_node->type == FUNCTION){
size_t buffer_size = 1024;
char *buffer = (char*) calloc(1, buffer_size);
FILE *tmp_out = fmemopen(buffer, buffer_size, "w+");
}
for (size_t i = 0; i < args_node->childs.len; i++) {
do_calls(ast, tmp_out, VEC_AT(&args_node->childs, i));
}
if (symbol_node->type == FUNCTION){
fflush(tmp_out);
fprintf(out, "%s", symbol_node->fp(buffer));
fclose(tmp_out);
free(buffer);
}

//TODO: deal with the error of not being able to open the library
}

struct extern_function* (*expose_library) () = dlsym(handle, "expose_library");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this could be a function pointer type, and also be exposed to the user in sseparate header file, along with extern_function.

vunit_run_vinumc_ok(ctx,
"[day_of_week 2024-09-26]\n",
&out,
"--with","subprojects/stdlib/libstdlib.so" , NULL
Copy link
Collaborator

Choose a reason for hiding this comment

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

hum, maybe create an external lib just for testing inisde the vinumc project. If you want to test the stdlib behavior (which we probably should) create a test dir on the stdlib project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that was the purpose of the stdlib of this pr

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, but I don't like that vinumc tests depend on another project.

Create another lib inside test for isolating the two projects. For now, it could be just a dumb function that returns some string, for the current tests it will suffice.

The stdlib part of this PR will be the starting point.

Copy link
Collaborator

@Grillo-0 Grillo-0 left a comment

Choose a reason for hiding this comment

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

Also, documentation stuff.

Add an entry to the list of subprojects on the ./README.md and also create a separate README.md for stdlib.

Also, we need a way to document the API used for creating an external lib. Maybe create a file on the docs folder showing how to do it?

Copy link
Collaborator

@Grillo-0 Grillo-0 left a comment

Choose a reason for hiding this comment

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

Also, let's start using the semantic versioning, bump vinumc version to 0.1.0.

@artP2 artP2 force-pushed the feat/16/dynamic-libraries branch 2 times, most recently from c405308 to 0e498bc Compare February 25, 2025 18:06
Add vinumc/extern_library.c and vinumc/extern_library.h which provide
structures and functions for loading external libraries using dlfcn.
@artP2 artP2 force-pushed the feat/16/dynamic-libraries branch 2 times, most recently from 6480550 to 261b4c2 Compare February 25, 2025 20:14
Gives the compiler the ability to load and execute external functions.
@artP2 artP2 force-pushed the feat/16/dynamic-libraries branch from 1bacd6c to e6e6d10 Compare February 25, 2025 20:31
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.

Dynamic load libraries for external functions
2 participants