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

Tracking PR for updated build method #1

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

safijari
Copy link

@safijari safijari commented Mar 4, 2019

So, I've set this up with a CMake file, made pybind11 into a submodule, etc. All this set up will let me build self contained wheels (manylinux) across a large number of python versions, which I'll be temporarily publishing to PyPI under lanms-proper. If you're interested in incorporating this and having CI set up to build and publish the wheels automatically please comment here and I can move this PR forward with those changes.

Copy link

@mristin mristin left a comment

Choose a reason for hiding this comment

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

Hi @safijari ,
Thanks a lot for the pull request! I have only a couple of minor questions which I'd like to clarify before we merge it in.

@@ -0,0 +1,3 @@
[submodule "pybind11"]
path = pybind11
url = https://github.com/pybind/pybind11.git
Copy link

Choose a reason for hiding this comment

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

Is there a way to pin the version of pybind? Otherwise, this module will break as soon as there are breaking changes introduced into pybind11.

Copy link
Author

Choose a reason for hiding this comment

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

Oh interesting, last time I added the submodule to another package it pinned it to a particular commit hash. I can do that

subprocess.check_call(['cmake', '--build', '.'] + build_args, cwd=self.build_temp)


setup(
Copy link

@mristin mristin Mar 4, 2019

Choose a reason for hiding this comment

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

Could you please update the following properties accordingly (or do you want me to update them)?

As far as I could see, there is no breaking change in the public interface, so we can simply bump the version to 1.0.3 (I'd do that in a separate pull request)?

Copy link
Author

Choose a reason for hiding this comment

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

Sure. I added them in haste as I needed something working for a project. Will update them accordingly and bump version.

@mristin
Copy link

mristin commented Mar 4, 2019

If you're interested in incorporating this and having CI set up to build and publish the wheels automatically please comment here and I can move this PR forward with those changes.

That would be great! Can we maybe do that in a separate pull request?

@safijari
Copy link
Author

safijari commented Mar 4, 2019

If you're interested in incorporating this and having CI set up to build and publish the wheels automatically please comment here and I can move this PR forward with those changes.

That would be great! Can we maybe do that in a separate pull request?

Sure. I've had decent luck with AppVeyor (e.g. https://github.com/safijari/apriltags2_ethz/).

I'd need you to set up your pypi credentials as a secret in appveyor or whatever we end up using. Either that or I'd need to be a collaborator for the lanms package on PyPI.

@mristin
Copy link

mristin commented Feb 24, 2021

@safijari would you be still interested to finish this pull request? I think this is related to the issue #6.

@mristin
Copy link

mristin commented Feb 24, 2021

@safijari

I'd need you to set up your pypi credentials as a secret in appveyor or whatever we end up using. Either that or I'd need to be a collaborator for the lanms package on PyPI.

It's probably most secure to set up pypi token credentials. Would appveyor still be your preferred platform?

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.

2 participants