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

PHP 7 Support #7

Merged
merged 10 commits into from Nov 22, 2015

Conversation

@Sean-Der
Copy link
Contributor

commented Jul 1, 2015

Hi!

This is a WIP branch for PHP 7 support. Currently all tests pass, but I still have some memory leaks I haven't addressed yet. I also need to mark a few spots that I have questions on. Would love some eyes on this to catch issues early on, thank you!

After that I am going to take swing fixing all the issues that show up with -Wall

@langemeijer

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2015

There is good work in this I can see, but there is one big issue: We need the ssh2 extension to compile to both php 5.6 and php 7.

@Sean-Der

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2015

Hey @langemeijer
Sorry for the late reply, not very hip with the new stuff...

So, what do you think about maintaining two branches? There is a fair amount of preprocessor stuff that we could rip out, and could take a chance to clean up the code base (-Wall found some stuff) This extension is also really stable, and hopefully we could just cherry-pick commits back and forth with minor difficulties.

I have been going this route with other extensions, but agreed to explore coming back and doing ZEND_ENGINE 2+3 compatibility with some maintainers.

Is it possible to push releases to pecl and prefix for versions of PHP? Does pickle make things any easier?

thanks!

@langemeijer

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2015

I don't mind maintaining two branches, but the underlying infrastructure for packaging is not really prepared for it. I'm primarily thinking of PECL in this respect, but linux distros will also have trouble doing packaging correctly (although they have it easier, as they pin on a PHP version anyway and just need to be told to use another branch)

Traditionally we've always used the preprocessor stuff, and that has always worked, but has caused massive clutter. We probably still support PHP4 in ssh2, and that's just sad.

Unless someone fixes PECL I see no way to get branches working. I think conditional compilation is the easiest way to go.

@langemeijer

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

@sergeyklay

This comment has been minimized.

Copy link

commented Oct 28, 2015

Any updates?

@Sean-Der

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2015

Hey @sergeyklay !

This should work with PHP 7 right now, are you having issues with it?

@langemeijer sorry for missing your follow up. For yaml/msgpack we ended up doing a PHP 7 only release, and it has worked out well.. so far!
I feel bad, but I just don't have the time/motivation to make this work for PHP 5. I just use PHP 7 (and have other PHP work that is nagging me).

I am totally happy if you final decision is a single branch, I can open another PR starting that. I think the extension would/should go through major refactors though (drop all the ZTS hackery) so it may not be a small undertaking.

thanks

@langemeijer

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2015

Hi Sean,

I was at my local coding conference, listening to this awesome talk by http://conference.domcode.org/talks.html#phintjens the talk is recorded, should be online soon.

We have to merge. You've done an awfull lot of work.

I think I found a way around the pecl problem: we make the new PHP7 pecl package an alpha release, the current one is a beta. To install the PHP7 package you would need to install ssh2-alpha , php5 ssh2-beta.

I will merge later today branching the current master into a php5 branch, the php7 code should live in master. Any news on the memory leaks you mentioned in the first comment?

@Sean-Der

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2015

@langemeijer The talk sounds really interesting! I am not so great with thinking ahead when it comes to OpenSource, if something excites me/I want it done I do it. However, I have created a fair amount of abandoned code that I could have done a better job of owning/managing. I get pulled away before finishing stuff.

There are some datatype + memory leak issues that I never addressed (and I only realized some errors as I spend more time doing php-src work!) I am going to fix this tonight, and will comment when it is 'good enough to merge'.

I would really like to improve the testing situation. In a perfect world.

  • Run the test suite on every commit, running a SSH server adds some complexity but we can do it!
  • Add code coverage, and write tests for all untested public functions.
  • Run in debug mode (to get zend memory manager stuff) + valgrind

Also, are you in any IRC/Slack channels for pecl stuff? I idle on efnet, and if you are waiting for me on anything would love to give you a better way to poke me.

thanks

@Sean-Der Sean-Der force-pushed the Sean-Der:php7 branch from e636786 to b08b041 Nov 20, 2015

Update zend_parse_parameter calls to use size_t + zend_long, fix memo…
…ry leak in ssh2_fingerprint, update .gitignore to ignore files from failed tests
@Sean-Der

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2015

Hey @langemeijer !

Sean-Der@9e77ed1 fixed the issues I was referring to. IMO it is good enough for a beta (I would like to hit everything in my TODO list above before I feel comfortable enough with a wide release and I need to cleanup comments+noop stuff from PHP 5)

thanks

@php-pulls php-pulls merged commit 9e77ed1 into php:master Nov 22, 2015

php-pulls pushed a commit that referenced this pull request Nov 22, 2015

Merge pull request #7
This merge causes the ssh2 pecl extension to compile against php7 only.
For php5 support see the php5 branch.
@sergeyklay

This comment has been minimized.

Copy link

commented Nov 22, 2015

👍

@acasademont

This comment has been minimized.

Copy link

commented Mar 17, 2016

Hi @Sean-Der thanks for the work! Could it be possible to realease another PECL tag? Right now we have to compile the extension manually. Thanks!

@langemeijer

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2016

A pecl release will follow in a few weeks.

On Thu, 17 Mar 2016 09:42 Albert Casademont, notifications@github.com
wrote:

Hi @Sean-Der https://github.com/Sean-Der thanks for the work! Could it
be possible to realease another PECL tag? Right now we have to compile the
extension manually. Thanks!


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#7 (comment)

@Jean85

This comment has been minimized.

Copy link
Contributor

commented May 4, 2016

Bump.
Still no PECL release? Are there any remaining bugs?

@Sohorev

This comment has been minimized.

Copy link

commented May 4, 2016

wait for pecl +1

@peteward

This comment has been minimized.

Copy link

commented May 4, 2016

indeed, quite desperate for a PECL release too!

@gonzalovilaseca

This comment has been minimized.

Copy link

commented May 6, 2016

Same here!

@langemeijer

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2016

Two PECL releases have been made.
0.13 (stable) is the package as built from the php5 branch
1.0 (alpha) is the package as built from the master branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
9 participants
You can’t perform that action at this time.