-
Notifications
You must be signed in to change notification settings - Fork 3
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
fixes #81, adding partial windows build support to aid in skywallet-mcu#342 #82
fixes #81, adding partial windows build support to aid in skywallet-mcu#342 #82
Conversation
@olemis PR ready to merge, please check the commits comments on the Github web for change descriptions |
@stdevPavelmc in a while , thnx |
Makefile
Outdated
|
||
install-protoc-win32: /usr/local/bin/protoc.exe | ||
|
||
#install-protoc-win64: /usr/local/bin/protoc.exe |
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.
What about 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.
The partial windows support is all about this, the partial part is just the support for install the protoc.exe tool in windows.
By the way it's handled in the Makefile (protoc install) the less invasive mode I found was to split the install-protoc target in specific OS ones, so there is a new install-protoc-win32
target that get's called only on windows. (also a new install-protoc-linux
and install-protoc-osx
was created...)
About the win32/win64 one, we use 64 bits when possible but the actual protob release we use (3.6.1) hasn't a 64 bit version so we identify it and install the 32 bit one.
The commented 64 bit target can be removed, it was left by accident (it came from the initial issue testing) sorry.
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.
Removing the commented win64 target via a commit in the way...
|
||
install-protoc-osx: install-protoc | ||
|
||
install-protoc-win32: /usr/local/bin/protoc.exe |
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.
Does this require Win Linux subsystem ?
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.
Nope, it's pointless to use WSL as it will generate a linux (elf) bin inside windows, it uses the MSYS2 toolchain (based on MINGW32/64) to generate a compatible .exe windows bin
We switch to appveyor (skywallet-mcu#342 original issue) because it can handle a clean MSYS2 toolchain in opposite of travis that use git-windows shell and mixing that with MSYS2 is a PITA and you get unstable results (remember the travis log generating garbage, a side effect of mixing git-win and MSYS2 toolchains on windows)
Also there are some libs we need that are only found in MSYS2.
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.
What is /usr/local/bin/protoc.exe
then ? It does not look like a Windows file path , right ?
…ied_to_master Apply changes from PR #82 to the develop [default] branch (aid on solution for fibercrypto/skywallet-mcu#342)
Fixes #81 adding partial windows build support with the possibility of install the protoc.exe in windows, as needed in fibercrypto/skywallet-mcu#342 (see Related issues below)
Changes:
install-protoc
by OS with dedicated PHONY entries to call the correct oneDoes this change need to mentioned in CHANGELOG.md?
No
Related issues:
fibercrypto/skywallet-mcu#342