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

fixes #81, adding partial windows build support to aid in skywallet-mcu#342 #82

Conversation

stdevPavelmc
Copy link
Contributor

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:

  • Makefile with some tweaks:
    • It now detects an additional OS: MINGW64 aka Windows
    • Split the target install-protoc by OS with dedicated PHONY entries to call the correct one

Does this change need to mentioned in CHANGELOG.md?
No

Related issues:
fibercrypto/skywallet-mcu#342

@stdevPavelmc
Copy link
Contributor Author

@olemis PR ready to merge, please check the commits comments on the Github web for change descriptions

@olemis
Copy link
Collaborator

olemis commented Dec 10, 2019

@stdevPavelmc in a while , thnx

Makefile Outdated

install-protoc-win32: /usr/local/bin/protoc.exe

#install-protoc-win64: /usr/local/bin/protoc.exe
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about this ?

Copy link
Contributor Author

@stdevPavelmc stdevPavelmc Dec 10, 2019

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.

Copy link
Contributor Author

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
Copy link
Collaborator

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 ?

Copy link
Contributor Author

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.

Copy link
Collaborator

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 ?

@stdevPavelmc stdevPavelmc requested a review from olemis December 10, 2019 16:48
@olemis olemis changed the title Refs #81, adding partial windows build support to aid in skywallet-mcu#342 fixes #81, adding partial windows build support to aid in skywallet-mcu#342 Dec 10, 2019
@olemis olemis added ci Tests and continuous integration os-windows Specific to Microsoft Windows platform stdev wip Work in progress labels Dec 10, 2019
@olemis olemis merged commit dca5f7c into fibercrypto:master Dec 10, 2019
olemis added a commit that referenced this pull request Dec 13, 2019
…ied_to_master

Apply changes from PR #82 to the develop [default] branch (aid on solution for fibercrypto/skywallet-mcu#342)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Tests and continuous integration os-windows Specific to Microsoft Windows platform stdev wip Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need to add partial Windows build support
2 participants