-
Notifications
You must be signed in to change notification settings - Fork 17
Use lib from other #13
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
Conversation
fuzzybinary
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot going on in this PR. Would you be able to separate it out into smaller, more self contained PRs? For example, just the build changes?
.github/workflows/build.yaml
Outdated
| - uses: actions/checkout@v3 | ||
| - uses: dart-lang/setup-dart@v1 | ||
| - uses: ilammy/msvc-dev-cmd@v1 | ||
| - name: Install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: name should probably be 'Install dependencies'
| "dart-sdk" | ||
| ] | ||
| ], | ||
| "files.associations": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it's more of a personal setting than a project setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"dart.analysisExcludedFolders": [
"dart-sdk"
]
This was hard too find without this you see millions from Warnings in VS Code. And not the Warnings of Code in this Lib. I don't how handle it.
| Dart_EnterScope(); | ||
| Dart_Handle root_library = Dart_RootLibrary(); | ||
| _root_library = Dart_NewPersistentHandle(root_library); | ||
| Dart_SetFfiNativeResolver(root_library, ResolveNativeFunction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be necessary. I'll try to take a look at what's going soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As expected, this is not needed now that -export-dynamic is being used.
src/CMakeLists.txt
Outdated
| ) | ||
|
|
||
| if(WIN32) | ||
| set_property(TARGET dart_dll PROPERTY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set_property(TARGET dart_dll PROPERTY
MSVC_RUNTIME_LIBRARY "MultiThreaded$<$CONFIG:Debug:Debug>")
The Line was Double
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Line is double
| #include <iostream> | ||
|
|
||
| #include "dart_dll.h" | ||
| #include "include/dart_dll.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather add include to the include path than add include here.
| if(LIB_DART_DEBUG) | ||
| target_link_libraries(dart_dll debug ${LIB_DART_DEBUG}) | ||
| else() | ||
| target_link_libraries(dart_dll debug ${LIB_DART_RELEASE}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the debug here as the second parameter correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes if you build only Release from dart as dll but this lib is build als debug. you get an Link error.
So you only build hier release in Pipeline you muss have this. So I Think.
| // Dart accessible funcitons | ||
| // These need to be exposed as "C" functions and be exported | ||
| // | ||
| #if defined(_WIN32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really see a reason to move these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can better read the Code when not all Functions in one File.
main Builds the app and run it.
exportfunc all Functions are Exported to Dart VM to used from there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I partially agree, but for now I'd like to leave it as is. It will also help keep the PR more focused on one feature -- adding / fixing build functionality.
If you want to move the exported functions, you can open a separate PR for it.
|
I'm closing this for lack of movement. |
I have many mirco Changes to Build and used it from an Other Project with CMake and FetchContent
https://github.com/DerKleinePunk/HostDartVM
I have Found the realtime_example is not Working on Linux the Problem is WorkFfiCalls find not the Nativ Functions.
When Build with Fetch CMake set RuntimeLibrary to MultiThreadedDebugDLL I don't know why I Can't Find it.
I see Warnings may be Ok
\libdartshared-src\dart-sdk\sdk\runtime\platform/globals.h(475,29): warning C4309: "Initialisierung":
\libdartshared-src\dart-sdk\sdk\runtime\platform/globals.h(478,31): warning C4309: "Initialisierung":
\libdartshared-src\dart-sdk\sdk\runtime\platform/utils.h(141,12): warning C4244: "return":
\libdartshared-src\dart-sdk\sdk\runtime\platform/hashmap.h(29,1): warning C4267: "Initialisierung":