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.

Sign up for GitHub

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

Jump to bottom

Only require FFI on Windows #132

Closed

DavidS wants to merge 1 commit into enkessler:dev from DavidS:optional-ffi-dependency

Contributor

DavidS commented Dec 20, 2017

This allows users to trade off between full-featured and
always-deployable by removing the hard dependency on a native extension.
The user of the gem needs to depend on FFI themselves to enable
posix_spawn on Linux.

Contributor Author

DavidS commented Dec 20, 2017

This is an alternative implementation to #127

coveralls commented Dec 20, 2017 •

edited

Loading

Coverage decreased (-11.1%) to 79.58% when pulling 1be5200 on DavidS:optional-ffi-dependency into ba46099 on enkessler:dev.

coveralls commented Dec 21, 2017 •

edited

Loading

Coverage decreased (-11.1%) to 79.58% when pulling 7bfaab8 on DavidS:optional-ffi-dependency into ba46099 on enkessler:dev.

8 similar comments

coveralls commented Dec 21, 2017

Coverage decreased (-11.1%) to 79.58% when pulling 7bfaab8 on DavidS:optional-ffi-dependency into ba46099 on enkessler:dev.

coveralls commented Dec 21, 2017

Coverage decreased (-11.1%) to 79.58% when pulling 7bfaab8 on DavidS:optional-ffi-dependency into ba46099 on enkessler:dev.

coveralls commented Dec 21, 2017

Coverage decreased (-11.1%) to 79.58% when pulling 7bfaab8 on DavidS:optional-ffi-dependency into ba46099 on enkessler:dev.

coveralls commented Dec 21, 2017

Coverage decreased (-11.1%) to 79.58% when pulling 7bfaab8 on DavidS:optional-ffi-dependency into ba46099 on enkessler:dev.

coveralls commented Dec 21, 2017

Coverage decreased (-11.1%) to 79.58% when pulling 7bfaab8 on DavidS:optional-ffi-dependency into ba46099 on enkessler:dev.

coveralls commented Dec 21, 2017

Coverage decreased (-11.1%) to 79.58% when pulling 7bfaab8 on DavidS:optional-ffi-dependency into ba46099 on enkessler:dev.

coveralls commented Dec 21, 2017

Coverage decreased (-11.1%) to 79.58% when pulling 7bfaab8 on DavidS:optional-ffi-dependency into ba46099 on enkessler:dev.

coveralls commented Dec 21, 2017

Coverage decreased (-11.1%) to 79.58% when pulling 7bfaab8 on DavidS:optional-ffi-dependency into ba46099 on enkessler:dev.

DavidS force-pushed the optional-ffi-dependency branch 2 times, most recently from 11f5a64 to a2e19c7 Compare

January 5, 2018 15:19


          Only require FFI on Windows

99f279b

This allows users to trade off between full-featured and
always-deployable by removing the hard dependency on a native extension.
The user of the gem needs to depend on FFI themselves to enable
`posix_spawn` on Linux.

For testing purposes, the Gemfile loads ffi if required by the environment.

DavidS force-pushed the optional-ffi-dependency branch from a2e19c7 to 99f279b Compare

January 5, 2018 15:44

Contributor Author

DavidS commented Jan 5, 2018

Hi @enkessler, apologies for the push spam, I relaized too late that I should have done that on a private branch before PR'ing.

Anyways, this should pass now, and make the FFI dependency optional on platforms that don't need it. This is much less invasive than #127, and still would fix my team's issues.

Happy New Year!

coveralls commented Jan 5, 2018

Coverage decreased (-0.3%) to 90.444% when pulling 11f5a64 on DavidS:optional-ffi-dependency into ba46099 on enkessler:dev.

1 similar comment

coveralls commented Jan 5, 2018 •

edited

Loading

Coverage decreased (-0.3%) to 90.444% when pulling 11f5a64 on DavidS:optional-ffi-dependency into ba46099 on enkessler:dev.

coveralls commented Jan 5, 2018 •

edited

Loading

Coverage decreased (-0.3%) to 90.444% when pulling a2e19c7 on DavidS:optional-ffi-dependency into ba46099 on enkessler:dev.

coveralls commented Jan 6, 2018 •

edited

Loading

Coverage decreased (-0.3%) to 90.444% when pulling 99f279b on DavidS:optional-ffi-dependency into ba46099 on enkessler:dev.

2 similar comments

coveralls commented Jan 6, 2018 •

edited

Loading

Coverage decreased (-0.3%) to 90.444% when pulling 99f279b on DavidS:optional-ffi-dependency into ba46099 on enkessler:dev.

coveralls commented Jan 6, 2018 •

edited

Loading

Coverage decreased (-0.3%) to 90.444% when pulling 99f279b on DavidS:optional-ffi-dependency into ba46099 on enkessler:dev.

Owner

enkessler commented Jan 11, 2018

@DavidS I'll take another look.

DavidS added a commit to DavidS/puppet-resource_api that referenced this pull request


          (maint) revert experimental split childprocess gem dependency

e69ca6b

This means that for now the gem is not really usable on linux puppet
agents, but it'll work for development without special casing.

Final resolution of this is still in progress at enkessler/childprocess#132 and https://tickets.puppetlabs.com/browse/PDK-580

DavidS mentioned this pull request

Split gem to avoid FFI dependency for core #127

Closed

sds mentioned this pull request

Maintainer needed #131

Closed

Collaborator

sds commented Jan 12, 2019 •

edited

Loading

Thanks for the pull request, @DavidS!

I made some minor adjustments in 576c249 given a few files have changed since this was first opened, but the tests all passed in Travis and AppVeyor.

Excited this will remove the ffi requirement by default. This has been merged onto master.

sds closed this

sds mentioned this pull request

Make FFI optional #124

Closed

Contributor Author

DavidS commented Jan 14, 2019

Thanks a lot for picking this up @sds.

And a big thanks to @enkessler for stewardship. I know how hard OSS maintenance can be on the soul.

sds mentioned this pull request

Cut 1.0.0 release #142

Merged

ahorek mentioned this pull request

cannot load such file -- ffi on windows #150

Closed

delphaber mentioned this pull request

Relax childprocess dependency version sds/overcommit#649

Closed

sds mentioned this pull request

Failed to install offline on Windows platform #158

Closed

sds added a commit that referenced this pull request


          Do not install ffi gem automatically on Windows platforms

67845b0

We originally introduced this logic in #132 without understanding all
the implications.

Issue #158 raised a challenge with this approach.

sds added a commit that referenced this pull request


          Do not install ffi gem automatically on Windows platforms

ca709e5

We originally introduced this logic in #132 without understanding all
the implications.

Issue #158 raised a challenge with this approach.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request


          sysutils/ruby-childprocess: update to 3.0.0

45666e0

Update ruby-childprocessto 3.0.0.


### Version 3.0.0 / 2019-09-20

* [#156](enkessler/childprocess#156 unused `rubyforge_project` from gemspec
* [#160](enkessler/childprocess#160): Remove extension to conditionally install `ffi` gem on Windows platforms
* [#160](enkessler/childprocess#160): Remove runtime dependency on `rake` gem

### Version 2.0.0 / 2019-07-11

* [#148](enkessler/childprocess#148): Drop support for Ruby 2.0, 2.1, and 2.2
* [#149](enkessler/childprocess#149): Fix Unix fork reopen to be compatible with Ruby 2.6
* [#152](enkessler/childprocess#152): Fix hangs and permission errors introduced in Ruby 2.6 for leader processes of process groups

### Version 1.0.1 / 2019-02-03

* [#143](enkessler/childprocess#144): Fix installs by adding `rake` gem as runtime dependency
* [#147](enkessler/childprocess#147): Relax `rake` gem constraint from `< 12` to `< 13`

### Version 1.0.0 / 2019-01-28

* [#134](enkessler/childprocess#134): Add support for non-ASCII characters on Windows
* [#132](enkessler/childprocess#132): Install `ffi` gem requirement on Windows only
* [#128](enkessler/childprocess#128): Convert environment variable values to strings when `posix_spawn` enabled
* [#141](enkessler/childprocess#141): Support JRuby on Java >= 9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment