-
Notifications
You must be signed in to change notification settings - Fork 12
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
Update to Dart 3.3.0 and little changes for Building on Windows #12
Update to Dart 3.3.0 and little changes for Building on Windows #12
Conversation
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.
Thanks for the contribution. A few minor questions and we can get things update to Dart 3.2.6
README.md
Outdated
@@ -27,7 +27,8 @@ Github Actions currently builds a Windows x64 `.dll`, A Linux x64 `.so`, and a m | |||
You need: | |||
* git | |||
* Dart 3+ | |||
* C++ build tools for your platform (Visual Studio, XCode, gcc, etc) | |||
* C++ build tools for your platform (Visual Studio, XCode, gcc, etc) | |||
* For Windows VS 2019 16.61 with 10.0.20348.0 SDK don't forget install Debugger Tools |
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 any version of Visual Studio 2019+ should work. I'm using 2022 I think....
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 is difficlut, I you have more than one IDE / Compiler on Machine
see dart_shared_libray\dart-sdk\sdk\build\vs_toolchain.py
That was my Build Problem at first time i get always an fiele not found from windows SDK.
wenn it found an 2019 it build with this and that it muss be 10.0.20348.0 as SDK
I don't know witch SDK version is used with 2022.
I want to say with 2019 it work only with this SDK
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.
Left a comment on the PR. The officially supported toolchains and what actually works are sometimes different :) I don't have 2019, I only have 2022, but if you look at setupEnv.ps1
I tell the script it's Visual studio 2019.
The key I think is just VS2019 and the SDK version you mentioned.
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 use setupEnv.ps1 only the DEPOT_TOOLS_WIN_TOOLCHAIN=0 is an must.
I have 2019 and 2022 Prof on my Machine
Don't think it is good Idea say there ist an 2019 and Complie with 2022.
I think we need an better description in readme vor that.
Sorry for my very Bad English i'm German |
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.
A few more comments, but mostly just nit picks.
Also don't worry about your English. Dein Englisch ist besser als mein Deutsch.
@@ -15,11 +15,12 @@ class BuildToolsLogger { | |||
} | |||
|
|||
static Logger initLogger({Level logLevel = Level.info}) { | |||
return Logger( | |||
_shared = Logger( |
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 this change?
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.
Please Debug your Code. Without this the shared Logger hast always the Level Info so the Init do not work
for other Logger called in Script. I have Teste with more Debug log and Can't see the Logoutput.
final logger = BuildToolsLogger.shared;
static Logger get shared {
_shared ??= initLogger();
return _shared!;
}
Add First Call shared ist null and creates new one with default level. so i Init in Init...
@@ -47,8 +66,21 @@ void main(List<String> args) async { | |||
exit(-1); | |||
} | |||
|
|||
if (!await _buildDart()) { | |||
exit(-1); | |||
if (buildType == "all") { |
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 there's a simpler way to do this. I'll take a stab at it and modify the PR this evening.
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.
Here's my stab at supporting multiple targets: https://github.com/fuzzybinary/dart_shared_libray/blob/987261ef05376b1d1dea0dbd4bbcab23ae5c9f86/scripts/build_helpers/bin/build_dart.dart#L12
logLevel: logLevel, | ||
); | ||
|
||
String buildType = argResults['buildType'] ?? "all"; |
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 the default should be release
not all
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.
Default working without ?? all i think. it looks on my tests now
target_link_libraries(simple_example PUBLIC dart_dll) | ||
|
||
if (MSVC) |
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.
Do you have to wrap this in MSVC? VS_DEBUGGER_WORKING_DIRECTORY
should silently fail for anything other than MSVC.
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.
No i build with VSCode but you have it on other samples why not in simple ?
There ist also an "Problem" realtime_example is the only sample wich is in the Debug folder all other samples only
in example<name>\ debug folder. I understand not why.
my practice is put the output too an bin folder i no that makes small count of developer it is easier to wirte launch.json for debugging
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.
Yeah, to be honest I'm not as good with CMake as I'd like, so I'm not surprised there are weird issues.
I'll leave this in if it's working for you and take another pass at the CMake files at some point.
* C++ build tools for your platform (Visual Studio, XCode, gcc, etc) | ||
* For Windows | ||
* 2019 16.61 with 10.0.20348.0 SDK don't forget install Debugger Tools | ||
* 2022 17 with ? SDK don't forget install Debugger Tools |
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 the windows SDK is always going to be the same, so I think the answer is VS 2019+ with Windows SDK 10.0.20348.0
I musst update Flutter Yesterday so im using now Dart 3.3 not more 3.2.6 i will hope that is ok |
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 looks good to me now. I'll run the github actions and then likely merge it in.
target_link_libraries(simple_example PUBLIC dart_dll) | ||
|
||
if (MSVC) |
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.
Yeah, to be honest I'm not as good with CMake as I'd like, so I'm not surprised there are weird issues.
I'll leave this in if it's working for you and take another pass at the CMake files at some point.
Going to merge this in, I'll look into the github action issues later tonight. Thanks for the contribution! |
Always Build an Debug Build for Testing with the dll