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

Add MacOS and Windows build targets #1293

Closed
wants to merge 17 commits into from

Conversation

StephenBrown2
Copy link
Contributor

Change Summary

As of about a week ago, the cibuildwheel repository has new examples for Travis on Mac and Windows: https://github.com/joerick/cibuildwheel/blob/master/examples/travis-ci-test-and-deploy.yml

This adds those targets to .travis.yml to hopefully resolve #555 .

If this passes (which I can't test myself, since you should handle the releases), I can edit https://github.com/samuelcolvin/pydantic/blob/8727914ced9d4546f0f95a453aff78f03b778275/docs/install.md#L19-L21 to reflect it.

Related issue number

#555

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

@codecov
Copy link

codecov bot commented Mar 5, 2020

Codecov Report

Merging #1293 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1293   +/-   ##
=======================================
  Coverage   99.89%   99.89%           
=======================================
  Files          21       21           
  Lines        3713     3725   +12     
  Branches      731      736    +5     
=======================================
+ Hits         3709     3721   +12     
  Misses          2        2           
  Partials        2        2           
Impacted Files Coverage Δ
pydantic/utils.py 98.98% <0.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 960b24a...f284b83. Read the comment docs.

@samuelcolvin
Copy link
Member

Humm, I'd rather move all this to github actions, see rtoml for a more complex but similar case.

Still until we move to github actions completely, maybe this would be a good stop gap.

@samuelcolvin
Copy link
Member

please confirm that these steps are working by commenting out the if: type = push AND (branch = master OR tag IS present) and deploy: chunks.

@StephenBrown2
Copy link
Contributor Author

Well, MacOS is building, but I can't figure out the Windows errors at the moment. I will happily leave those commented out for someone else to tackle, if you wish.

@StephenBrown2 StephenBrown2 mentioned this pull request Mar 11, 2020
4 tasks
@demospace
Copy link

demospace commented Mar 13, 2020

Well, MacOS is building, but I can't figure out the Windows errors at the moment. I will happily leave those commented out for someone else to tackle, if you wish.

I patched pydantic in a few places recently and tried to compile in Windows and ran into the following:

\temp.win-amd64-3.7\Release\pydantic\__init__.cp37-win_amd64.lib
LINK : error LNK2001: unresolved external symbol PyInit___init__
build\temp.win-amd64-3.7\Release\pydantic\__init__.cp37-win_amd64.lib : fatal error LNK1120: 1 unresolved externals
error: command 'C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\BuildTools\\VC\\Tools\\MSVC\\14.16.27023\\bin\\HostX86\\x64\\link.exe' failed with exit status 1120

The issue relates to distutils/setuptools and you can read more about it here:
https://bugs.python.org/issue35893
https://stackoverflow.com/questions/58797673/how-to-compile-init-py-file-using-cython-on-window?answertab=votes#tab-top

The SO answer is best. Adding the following to pydantic's setup.py resolves the build on Win32

from distutils.command import build_ext

def get_export_symbols_fixed(self, ext):
    pass

build_ext.get_export_symbols = get_export_symbols_fixed

(a previous version of this post provided the code from the first example which is fragile compared to the solution above)

@StephenBrown2
Copy link
Contributor Author

In theory this could break non-windows? So best to include it in an os.name check, like:

import os
from distutils.command import build_ext

if os.name == 'nt':

    def pass_ges(self, ext):
        pass

    build_ext.get_export_symbols = pass_ges

@samuelcolvin
Copy link
Member

In theory this could break non-windows? So best to include it in an os.name check, like:

import os
from distutils.command import build_ext

if os.name == 'nt':

    def pass_ges(self, ext):
        pass

    build_ext.get_export_symbols = pass_ges

agreed, let's try this.

@StephenBrown2
Copy link
Contributor Author

StephenBrown2 commented Mar 18, 2020

Doesn't looke like it's fixed that issue:

  LINK : error LNK2001: unresolved external symbol PyInit___init__

still happening. https://travis-ci.org/github/samuelcolvin/pydantic/jobs/664032568#L228

setup.py Outdated Show resolved Hide resolved
@StephenBrown2
Copy link
Contributor Author

Well it looks like that indeed worked. I took the function from cpython master and the patch from the bug report, and Windows builds are now succeeding!

@samuelcolvin
Copy link
Member

incorporated into #1326, thank you so much.

@StephenBrown2 StephenBrown2 deleted the add_mac_win_wheels branch May 1, 2020 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MacOS and windows pre-compiles binaries
3 participants