-
Notifications
You must be signed in to change notification settings - Fork 238
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
Added example using Buck with Visual Studio Code on Windows #517
Conversation
Thanks! I think our bar for samples is relatively low (if this is what you want to do, its super helpful to get started) so would be happy to get this included. |
0a8530f
to
c84b305
Compare
4781b1e
to
5d6f267
Compare
5d6f267
to
40b1867
Compare
Now I think it's ready for merging. Let me know why the "Meta Import Checks" is failing and what I can do to fix it. |
40b1867
to
a7a997b
Compare
This makes it impossible to choose the debug version of the run-time library, such as with /MDd for example. Even without any of the /MD, /MT or /LD options the linker doesn't seem to complain about missing symbols, presumably because the compiler defaults to one of these options.
6f3672e
to
79cec6c
Compare
Hello @ndmitchell, can you let me know why the "Meta Import Checks" fails? Is it the .vscode files? |
@Overhatted yes, that is why. I'll get that fixed, in the meantime I'm trying to see if I can get someone on the windows side internally to review this, otherwise Neil is right that the bar for examples is fairly low and we can just get this merged |
@JakobDegen has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
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.
Looks good to me, thank you!
@@ -67,7 +67,6 @@ def _system_cxx_toolchain_impl(ctx: AnalysisContext): | |||
shared_library_name_default_prefix = "" | |||
shared_library_name_format = "{}.dll" | |||
shared_library_versioned_name_format = "{}.dll" | |||
additional_linker_flags = ["msvcrt.lib"] |
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 one was added to make hello_world example to work on Windows. Do you know how does it build now?
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 can't think of any good reason. I tried to reproduce a failure like that with reasonable flags but couldn't get that error. Like it says in the documentation https://learn.microsoft.com/en-us/cpp/build/reference/md-mt-ld-use-run-time-library?view=msvc-170 the compiler places a linking directive in the object file specifying the runtime library to use (by default MT_StaticRelease on my machine).
If you pass in this flag https://learn.microsoft.com/en-us/cpp/build/reference/zl-omit-default-library-name?view=msvc-170 you do get that error but I did a CTRL+F through the repository and didn't find that flag anywhere.
@JakobDegen has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: 1. I made the default platform in the prelude public so it can be inherited from. I don't know why it wasn't already like that. Maybe I'm misunderstanding how to use it 2. Not sure if I'm using the config_setting and constraint_setting properly since it seems duplicated. 3. The script to generate the compile_commands.json was more or less copied from facebook/buck2#510. There are still a lot of improvements to be made but I think this is good enough to be merged: 1. With MSVC the compilation should probably be done with /Z7 by default and then, if the "stripped" sub-target is asked for, that flag is removed. 2. The cxx toolchain needs the compilation options looked at, not sure if Linux's defaults work properly. 3. The Install.ps1 and Copy.ps1 need to be ported to Linux. X-link: facebook/buck2#517 Reviewed By: KapJI Differential Revision: D53692733 Pulled By: JakobDegen fbshipit-source-id: 8d5962084831a3cccde128f0aa0360a1aaf72004
Summary: 1. I made the default platform in the prelude public so it can be inherited from. I don't know why it wasn't already like that. Maybe I'm misunderstanding how to use it 2. Not sure if I'm using the config_setting and constraint_setting properly since it seems duplicated. 3. The script to generate the compile_commands.json was more or less copied from facebook/buck2#510. There are still a lot of improvements to be made but I think this is good enough to be merged: 1. With MSVC the compilation should probably be done with /Z7 by default and then, if the "stripped" sub-target is asked for, that flag is removed. 2. The cxx toolchain needs the compilation options looked at, not sure if Linux's defaults work properly. 3. The Install.ps1 and Copy.ps1 need to be ported to Linux. X-link: facebook/buck2#517 Reviewed By: KapJI Differential Revision: D53692733 Pulled By: JakobDegen fbshipit-source-id: 8d5962084831a3cccde128f0aa0360a1aaf72004
There are still a lot of improvements to be made but I think this is good enough to be merged: