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

chore: Bundle all Ruby standalone binaries into wheel when deploying #256

Merged
merged 1 commit into from
Aug 22, 2021

Conversation

taj-p
Copy link
Contributor

@taj-p taj-p commented Aug 19, 2021

This pull request modifies setup.py to allow for offline installation of this package.

In a corporate environment, it is very difficult to garner adoption of a product that is difficult to install in an offline environment. This PR hopes to smooth out that process.

Users can now install this package via pip install pact-python --bin-path=/../../../ to prevent the installation process from trying to fetch binaries from github.com.

This seems to be a short term solution to #116. I am not sure whether the maintainers would want to close that issue or keep it open for the time being. As per @mefellows, "... the ideal state I think is that Python bundles the Ruby application into the package, so at install time no network is required." But this is planned for the FFI interface, which is still in development.

Also, this is my first time working within the pact-foundation, so please excuse any mistakes / issues I unintendedly may have made.

@mefellows
Copy link
Member

Thanks @fresto32 !

I think this PR is helpful, but I think a more helpful one would be to package the standalone into the pip package - is that possible?

With the proposed approach, you now need to sync an offline package (the standalone) with the pip package. Ideally, the whole package can be self contained.

@taj-p
Copy link
Contributor Author

taj-p commented Aug 20, 2021

Wow that was a quick turnaround. Thanks - yeah, if you're happy to bundle the current PACT_STANDALONE_VERSION standalones into the pip package, I'm more than happy to implement it! Stay tuned :)

@mefellows
Copy link
Member

I think that would be preferable. Elliot/Matt may have other ideas though?

Install all ruby standalone packages when running sdist.

Devloppers can also now provide a --bin-path that contains the binaries required
for their OS, so that can they use this package in offline environments.
@taj-p
Copy link
Contributor Author

taj-p commented Aug 20, 2021

We've made some adjustments to setup.py:

  • subclassed sdist so we can download all binaries into pact/bin when running python3 setup.py sdist, so our users don't need to fetch the binaries if they install the wheel artifact from pypi.
  • Kept the --bin-path for local development purposes
  • Still allow the downloading of artifiacts for local development purposes (i.e. you can still check out this repo, run make deps, and a fresh binary will be downloaded into the environment).

Please let me know if there's anything else we can do

@taj-p taj-p changed the title chore: Add offline installation of this package. chore: Bundle all Ruby standalone binaries into wheel when running python setup.py sdist Aug 20, 2021
@taj-p taj-p changed the title chore: Bundle all Ruby standalone binaries into wheel when running python setup.py sdist chore: Bundle all Ruby standalone binaries into wheel when deploying Aug 20, 2021
@mefellows
Copy link
Member

Awesome, thanks so much! I'll leave it to the maintainers of pact-python to review.

if download_bin_path is not None:
if os.path.isfile(path) is not True:
raise RuntimeError('Could not find {} binary.'.format(path))
extract_ruby_app_binary(download_bin_path, package_bin_path, binary['filename'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line reachable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this code will be hit if the user supplies a bin-path (i.e. download_bin_path) CLI argument and it contains the necessary standalone binaries

Copy link
Contributor

Choose a reason for hiding this comment

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

@fresto32 please note that in line 108 we check the opposite condition, so therefore download_bin_path is not None will never be false. I think the current behavior is broken, when I try to install, the binary is not downloaded currently. It worked for me after I removed the if and left:

download_ruby_app_binary(download_bin_path, binary['filename'], binary['suffix'])
extract_ruby_app_binary(download_bin_path, package_bin_path, binary['filename'])

@elliottmurray
Copy link
Contributor

Sorry for being awol. This is looking pretty good. Thank you for the awesome contribution! Had one minor comment but I'm going to merge anyway.

@elliottmurray elliottmurray merged commit f2230b6 into pact-foundation:master Aug 22, 2021
@taj-p
Copy link
Contributor Author

taj-p commented Aug 22, 2021

Thanks Elliot! 🥳 Feels great to contribute to Pact.

arturoriter added a commit to OutSystems/pact-python that referenced this pull request Aug 25, 2021
* chore: Releasing version 1.4.0

* fix: make uvicorn versions over 0.14 (pact-foundation#255)

* chore: Releasing version 1.4.1

* chore: Bundle Ruby standalones into dist artifact. (pact-foundation#256)

Install all ruby standalone packages when running sdist.

Devloppers can also now provide a --bin-path that contains the binaries required
for their OS, so that can they use this package in offline environments.

* chore: Releasing version 1.4.2

Co-authored-by: Elliott Murray <elliottmurray@gmail.com>
Co-authored-by: Taj Pereira <tajpereira@gmail.com>
arturoriter pushed a commit to OutSystems/pact-python that referenced this pull request Sep 3, 2021
Install all ruby standalone packages when running sdist.

Devloppers can also now provide a --bin-path that contains the binaries required
for their OS, so that can they use this package in offline environments.
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.

4 participants