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

php7 branch? #40

Closed
rlerdorf opened this Issue Mar 23, 2015 · 74 comments

Comments

Projects
None yet
@rlerdorf
Copy link

rlerdorf commented Mar 23, 2015

Could you make a php7 branch so we can PR against it to start the PHP 7 port?

@peterbowey

This comment has been minimized.

Copy link

peterbowey commented Mar 23, 2015

Good suggestion, 'imagick' has a working PHP 7 branch: https://github.com/mkoppanen/imagick/tree/phpseven

@tricky

This comment has been minimized.

Copy link
Member

tricky commented Mar 23, 2015

Certainly, anything we can do to help! php7-dev is in.

@pierrejoye

This comment has been minimized.

Copy link
Member

pierrejoye commented Mar 24, 2015

We could but I really do not like this branch thing, everyone uses a different name, let alone the pain from a semver point of view.

@tricky what's about simply different files? So every thing is synced? I am sure we will have x.y.z with the same features for both 5.x and 7.x, it will be easier to make changes in one branch and release in one go. Either #ifdef or config time inclusion will do it. Thoughts?

@tricky

This comment has been minimized.

Copy link
Member

tricky commented Mar 24, 2015

Neither do I. I very much prefer keeping one master branch and place version specifics in separate files as much as possible. It's simply too tempting to diverge branches. An absolutely, only one branch for releases.

However, clean separation and branching really are independent. If a branch brings peace of mind to a contributor, then why not? It's not like we have that many. :)

@rlerdorf

This comment has been minimized.

Copy link
Author

rlerdorf commented Mar 24, 2015

Once we have a working version, you can merge and ifdef or split into files. Whatever you like. But branches are really best for work-in-progress stuff like this. This is not going to be a trivial change. It is going to take a couple of people a bit of time to get right. I would much prefer to not break or in any way interfere with any upcoming production releases while we work on this.

@tricky

This comment has been minimized.

Copy link
Member

tricky commented Mar 24, 2015

I agree.

Can you shed some light on the changes we're looking at? How much are zval and object structures in flux?

@phadej

This comment has been minimized.

Copy link
Member

phadej commented Mar 24, 2015

The travis now tries to build with php7 too: https://travis-ci.org/igbinary/igbinary/jobs/55634363

The error doesn't look very nice, maybe there are very radical changes so keeping both php5 and php7 versions in one codebase might turn out to be quite tricky.

I found this thread http://thread.gmane.org/gmane.comp.php.devel/93164, yet currently I have no time to read it thru.

@rlerdorf

This comment has been minimized.

Copy link
Author

rlerdorf commented Mar 24, 2015

igbinary is a bit of a special case because it does session serialization and that api has changed quite a bit. Other than that it is mostly just zval** -> zval* and char* -> zend_string* changes and zend_hash looping and lookup type changes. We can also remove all the TSRM_* macros, although that is optional since PHP7 just sets them to nothing.

@centminmod

This comment has been minimized.

Copy link

centminmod commented Jun 14, 2015

curious how the PHP 7 updates going for PHP 7.0.0 Alpha 1 ?

@edtechd

This comment has been minimized.

Copy link

edtechd commented Jun 28, 2015

Hello,

How do you compile this branch? I found #include "zend_dynamic_array.h" in the igbinary.c but php7 source doesn't contain zend_dynamic_array functions. How to handle this issue?

Thanks.

@diemuzi

This comment has been minimized.

Copy link

diemuzi commented Jul 16, 2015

Curious how we can generate some more interest in getting this project updated for PHP 7? PHP 7-Beta 1 was recently released but I'm not yet able to get igbinary to compile. I'm trying to hack my way through it but so far coming up with snake eyes.

@Jan-E

This comment has been minimized.

Copy link

Jan-E commented Jul 17, 2015

Hi Pierre,

We could but I really do not like this branch thing, everyone uses a different name, let alone the pain from a semver point of view.

The branch thing is killing. A little list:

  1. phpng (propro, raphf, http)
  2. phpseven (imagick)
  3. php7 (redis, msgpack, excel)
  4. PHP7 (leveldb)
  5. php7-dev (igbinary)
  6. seven (pthreads)
  7. master for PHP7 and five for PHP5 (krakjoe/strict)

Ouch.

@rlerdorf

This comment has been minimized.

Copy link
Author

rlerdorf commented Jul 17, 2015

@Jan-E If the different branch names is your biggest problem with it, agreeing on one name and getting people to do a git branch -m seems a hell of a lot easier than trying to #ifdef in the changes. For simple extensions, that can work, but for the more involved ones it gets extremely messy and unreadable.
And I would agree with @krakjoe that eventually it is the PHP 5 version that should be on a branch and master should be for the current version of PHP. He is probably just a bit early with that.

@Jan-E

This comment has been minimized.

Copy link

Jan-E commented Jul 17, 2015

I once proposed master vs phpng (5-7) and master vs phpog (7-5), if a developer really has to use 2 branches. That was back in the days when @m6w6 introduced phpng.

@tricky

This comment has been minimized.

Copy link
Member

tricky commented Jul 17, 2015

Can't say how much work this will be, but I'll take a closer look once I get back home next week. Unless @phadej beats me to it? :)

@pierrejoye

This comment has been minimized.

Copy link
Member

pierrejoye commented Jul 18, 2015

The branching name is a little and easy to solve problem.

I am not sure branches are good from mid term point of view. It is not
semver and is pain to maintain (user exposed APIs wised f.e.).

I understand it is easier to do right now to test ports to 7 but as the
core APIs should be rather stable now, we could move to dual files or
simply same files with ifdef asap (for simple exts, igbinary is not in this
category).
On Jul 18, 2015 3:22 AM, "Rasmus Lerdorf" notifications@github.com wrote:

@Jan-E https://github.com/Jan-E If the different branch names is your
biggest problem with it, agreeing on one name and getting people to do a
git branch -m seems a hell of a lot easier than trying to #ifdef in the
changes. For simple extensions, that can work, but for the more involved
ones it gets extremely messy and unreadable.
And I would agree with @krakjoe https://github.com/krakjoe that
eventually it is the PHP 5 version that should be on a branch and master
should be for the current version of PHP. He is probably just a bit early
with that.


Reply to this email directly or view it on GitHub
#40 (comment).

@acrolink

This comment has been minimized.

Copy link

acrolink commented Aug 3, 2015

Would there be a release for PHP 7 in the near future?

I have installed PHP 7 beta 2 but its serialize function doesn't produce a functional string that can be unserializable for one of my big objects. Looking for alternatives already.

@tricky

This comment has been minimized.

Copy link
Member

tricky commented Aug 3, 2015

I have a beta-beta-hack for 7 on its way without session handling, will still take a few days though.

@Jan-E

This comment has been minimized.

Copy link

Jan-E commented Aug 4, 2015

Do you have a repository somewhere with your changes? I'd try if it compiles under Windows (with VS 2015).

@tricky

This comment has been minimized.

Copy link
Member

tricky commented Aug 4, 2015

I'll push them over here in a separate branch once done.

@Sean-Der

This comment has been minimized.

Copy link

Sean-Der commented Aug 12, 2015

Hey everyone, I started on this with PR #46

Currently only builds without APC/APCU and has plenty of test fails, but will be working on bringing those down today.

@phadej

This comment has been minimized.

Copy link
Member

phadej commented Aug 12, 2015

I propose we start totally new repository for php7 version, it's quite a fork, probably will have totally separate issues (and motivation to develop). There are two options igbinary/igbinary2 or igbinary/igbinary7.

@tricky said he will have preview very soon now ™️ , it's unfortunate that @Sean-Der did some work too :(

Another reason for separate repository: I don't know how to drop forked from somia/igbinary (which is forked from phadej/igbinary*.

@Sean-Der

This comment has been minimized.

Copy link

Sean-Der commented Aug 12, 2015

@phadej I am totally happy to drop my fork once @tricky's is done, I just spent a couple hours on it!

I have been avoiding IFDEFs so far, my hope is that something like pickle will make it easier to maintain concurrent branches. With msgpack I have just been cherry picking and git cherry shows me right away the things that haven't been pulled across.

The code just ended up being buggier / taking more time to develop with so many macros.

@Jan-E

This comment has been minimized.

Copy link

Jan-E commented Aug 12, 2015

Either way, ig_win32.h had to be fixed. I created this PR:
Sean-Der#1
Sean-Der cherry-picked it.

@phadej

This comment has been minimized.

Copy link
Member

phadej commented Aug 12, 2015

@Sean-Der I'd avoid separate branches, they are rather inconvenient when they are both "masters". Rather have totally separate repositories, with readmes explaining the situation. Except if you have really good reasons why we shouldn't do so. igbinary for PHP5 was practically stable for last many years (except e.g. windows :( ), and it's probably easier just to manually copy the test cases and such.

@phadej

This comment has been minimized.

Copy link
Member

phadej commented Aug 12, 2015

@Jan-E sorry for the windows situation, we just don't have windows machines :(

I try to figure out something, asap ™️.

@Sean-Der

This comment has been minimized.

Copy link

Sean-Der commented Aug 12, 2015

@phadej Nope, I think you are totally right with doing separate repos!
I will keep committing against this branch for now, but when you have the time to make a new repo etc... I will change things as needed on my end (but hopefully I am done with porting today!)

thanks.

@acrolink

This comment has been minimized.

Copy link

acrolink commented Aug 12, 2015

Great job guys. Good luck and happy programming. Just tell me what do I need to compile it under Windows and I will happily test the code.

@Jan-E

This comment has been minimized.

Copy link

Jan-E commented Aug 13, 2015

@acrolink My PHP7 builds now contain php_igbinary.dll's:
https://www.apachelounge.com/viewtopic.php?t=6617

Example of a phpinfo():
https://phpdev.toolsforresearch.com/php-7.0.0beta3-Win32-VC14-x64.htm

Note: for redis I am using the repository of @edtechd
It is known to have issues.

Note as well, that @Sean-Der says there were still has plenty of test fails in his fork of igbinary.

@phadej Do not worry about the Windows builds.

@Techmind

This comment has been minimized.

Copy link

Techmind commented Dec 9, 2015

Tnx for your work guys, but php7 version is bugged:

Used commit 2880c3e from php7-dev-playground1.
it works okay for cli, but does some heavy memory corruption for fpm+opcache enabled (took me 6 hours of gdb and patching php to find out, that problem was outside of php-src itself).

Basicly somehow extension manages to write to
p->next_free_slot value of (0x4 or 0x3 or 0x1) in zend_mm_heap *heap
see -> Zend/zend_alloc.c: function zend_mm_alloc_small_slow

This function is the only place there assignment to this variable happens (in php-src)
-[~/php7/php-src]-[git (php-7.0.0)]
$ git grep ">next_free_slot ="
Zend/zend_alloc.c: p->next_free_slot = (zend_mm_free_slot_)((char_)p + bin_data_size[bin_num]);;
Zend/zend_alloc.c: p->next_free_slot = NULL;
Zend/zend_alloc.c: p->next_free_slot = heap->free_slot[bin_num];

(ignore zend_mm_free_small -> there is just copying of already assigned value), but corrupted pointer is never assigned in this function: i added something like this

if (((int)p < 20 && (int)p > 0) || ((int)(p->next_free_slot) < 20 && (int)(p->next_free_slot) > 0)) {
zend_test_mem_my_aaa((int) p);
}
to verify that broken value is not assigned by php-src itself, and added breakpoint for zend_test_mem_my_aaa -> it was never called.

php-src commit is - 60fffd296abce5fc071f3c173c25a2696cf683c6 (7.0.0 release)

After i commented out usage of igbinary_unserialize from my code -> problem disappeared.

Edit: i was unseriazling quite heavy object with 3-4 levels of nesting arrays/maps.

Edit2: used 32 bit os (ubuntu 14.04) (my vm is using it by default) to built everything from source.

@Jan-E

This comment has been minimized.

Copy link

Jan-E commented Dec 9, 2015

Try Sean-Der's PHP7 branch to see if it has the same problems:
https://github.com/Sean-Der/igbinary/tree/php7

@Techmind

This comment has been minimized.

Copy link

Techmind commented Dec 9, 2015

same:

/home/vagrant/php7/usr/bin/phpize && ./configure --with-php-config=/home/vagrant/php7/usr/bin/php-config && make && make install
....
Libraries have been installed in:
/home/vagrant/serdean/igbinary/modules

[12:44 PM]-[vagrant@homestead]-[~/serdean/igbinary]-[git php7]
$ cd ~/

$ ./run-fpm7.sh
[09-Dec-2015 12:45:14] NOTICE: [pool www] 'user' directive is ignored when FPM is not running as root
[09-Dec-2015 12:45:14] NOTICE: [pool www] 'group' directive is ignored when FPM is not running as root
[09-Dec-2015 12:45:14] NOTICE: fpm is running, pid 15462
[09-Dec-2015 12:45:14] NOTICE: ready to handle connections
[09-Dec-2015 12:45:32] WARNING: [pool www] child 15463 exited on signal 11 (SIGSEGV - core dumped) after 18.058482 seconds from start
[09-Dec-2015 12:45:32] NOTICE: [pool www] child 15590 started

$ gdb ~/php7/usr/sbin/php-fpm -c /tmp/core
....
#0 0x08537279 in zend_mm_alloc_small (bin_num=12, size=140, heap=0xb5a00040)
at /home/vagrant/php7/php-src/Zend/zend_alloc.c:1305
1305 heap->free_slot[bin_num] = p->next_free_slot;
(gdb) bt
#0 0x08537279 in zend_mm_alloc_small (bin_num=12, size=140, heap=0xb5a00040)
at /home/vagrant/php7/php-src/Zend/zend_alloc.c:1305
#1 zend_mm_alloc_heap (size=140, heap=0xb5a00040)
at /home/vagrant/php7/php-src/Zend/zend_alloc.c:1384
#2 zend_mm_realloc_heap (heap=0xb5a00040, ptr=0xb5bb6600, size=140,
copy_size=140) at /home/vagrant/php7/php-src/Zend/zend_alloc.c:1662

(gdb) p p
$1 = (zend_mm_free_slot *) 0x4
(gdb) q

@Jan-E

This comment has been minimized.

Copy link

Jan-E commented Dec 9, 2015

Pity. @Sean-Der Any idea?

@Techmind

This comment has been minimized.

Copy link

Techmind commented Dec 9, 2015

@Jan-E maybe bug is in opcache itself, and i just manage to trigger while using igbinary_code (additional memory allocations for example) will trying to create a small test to reproduce it a bit later.

@Sean-Der

This comment has been minimized.

Copy link

Sean-Der commented Dec 9, 2015

Hey @Jan-E thanks for pinging me!

I will run the test suite through valgrind today, and see if I can find an invalid read/writes sounds like a fun one to debug :)

sorry about the silence about fixing the reference tests, I need to learn the igbinary format before I was just fixing code I knew. I have a handful of pecl extensions that need merged upstream, so just working on that.

thanks

@Jan-E

This comment has been minimized.

Copy link

Jan-E commented Dec 9, 2015

I have a handful of pecl extensions that need merged upstream, so just working on that.

I know. I am building https://phpdev.toolsforresearch.com/php-7.0.0-Win32-VC14-x64.htm with some of your forks.

@Techmind

This comment has been minimized.

Copy link

Techmind commented Dec 10, 2015

@Jan-E @Sean-Der
Ahh, sorry guys.
I rebuild php with valgrind support today, it was other extension which was doing something wrong. Author of this extension fixed it in 5 minutes (i sent valgrind log to his facebook =)).

Sorry for misinforming) Actually igbinary seems to work)
Tnx for your work & time.

@razvanphp

This comment has been minimized.

Copy link

razvanphp commented Dec 10, 2015

I created a PR #54 for @Sean-Der branch, this breaks with php7.

I don't know about the other failed tests, maybe they are the cause, but for running the Yii framework, calling the magic setter on private attributes breaks everything with igbinary.

@Ben749

This comment has been minimized.

Copy link

Ben749 commented Dec 16, 2015

Sean-der version compiles and loads .. nevertheless even if I'm logging the unserialized version vs original one, haven't seen one error yet ..

@farmani

This comment has been minimized.

Copy link

farmani commented Dec 17, 2015

@razvanphp did you resolve the issue with yii framework? I have exact same issue

@razvanphp

This comment has been minimized.

Copy link

razvanphp commented Dec 17, 2015

No, there is nothing we can fix, igbinary needs to be fixed to work with php7, currently there are failed tests. I also added another one for this problem, it's in pull request but it's also ignored completely.

It's a shame this new php release does not trigger enough interest, maybe we should look at alternatives like msgpack instead.

@phadej

This comment has been minimized.

Copy link
Member

phadej commented Dec 17, 2015

I made a new repository to concentrate only on PHP7. Issue is igbinary/igbinary7#1

@phadej phadej closed this Dec 17, 2015

@pierrejoye

This comment has been minimized.

Copy link
Member

pierrejoye commented Dec 17, 2015

I am really not sure it is the right way to deal with 7 (or any other
version)

That's why branches exist :)

It is also a semantic version mess and very hard to deal with releases as
well.

Any thoughts about why not using branches instead?
On Dec 17, 2015 5:17 PM, "Oleg Grenrus" notifications@github.com wrote:

I made a new repository to concentrate only on PHP7. Issue is
igbinary/igbinary7#1 igbinary/igbinary7#1


Reply to this email directly or view it on GitHub
#40 (comment).

@farmani

This comment has been minimized.

Copy link

farmani commented Dec 17, 2015

@pierrejoye if its a backward compatibility issue I think its not possible to have same repo for both version and if users should consider which version should install (as other php extension that they need to install php7.0-ext beside php5-ext) so yes haveing a new different repo completely make sence to me.

@krakjoe

This comment has been minimized.

Copy link

krakjoe commented Dec 17, 2015

@pierrejoye I agree, not sure why there are three branches and now another repo for PHP 7 ... a branch would have been fine, but whatever, progress is progress ...

@razvanphp

This comment has been minimized.

Copy link

razvanphp commented Dec 17, 2015

I remember reading somewhere that they wanted to get rid of forked from somia/igbinary so probably now is the perfect moment.

@phadej

This comment has been minimized.

Copy link
Member

phadej commented Dec 17, 2015

You can still cherry-pick independent commits from one repository to another, e.g. adding single test igbinary/igbinary7@21e4be9

About versioning, the php7 release, would probably bump the version of package to 2.0.0 (while still being igbinary), and also internal format version (even we don't change the format itself).

I'll revisit what is in the two php7 branches in this repository, transfer relevant bits, and remove them.

@phadej

This comment has been minimized.

Copy link
Member

phadej commented Dec 17, 2015

Ok, seems I merged a PR into wrong branch (I planned to merge it to master) and have to revisit @tricky's commit in https://github.com/igbinary/igbinary/tree/php7-dev-playground2

It seems that igbinary7 doens't compile for me (I don't know how it compiles on travis - suspicious); will check that during weekend.

But please, from now on, PHP7 related discussion to other repository. Thanks!

@pierrejoye

This comment has been minimized.

Copy link
Member

pierrejoye commented Dec 17, 2015

I understand the need of separate trees but another repository is a mistake
imho.

I would suggest to use master for 7 and move 5.x to a branch.

Having two branches do not mean you have to merge everything but it makes
releases and versions management and tracking way easier.

Also if you like to use pickle at some point, it will make your whole work
easier as it uses branches/tags to grab releases (composer).

I apologize to insist but I would strongly suggest not to create another
repository.
On Dec 17, 2015 8:30 PM, "Oleg Grenrus" notifications@github.com wrote:

You can still cherry-pick independent commits from one repository to
another, e.g. adding single test igbinary/igbinary7@21e4be9
igbinary/igbinary7@21e4be9

About versioning, the php7 release, would probably bump the version of
package to 2.0.0 (while still being igbinary), and also internal format
version (even we don't change the format itself).

I'll revisit what is in the two php7 branches in this repository, transfer
relevant bits, and remove them.


Reply to this email directly or view it on GitHub
#40 (comment).

@phadej

This comment has been minimized.

Copy link
Member

phadej commented Dec 17, 2015

Sorry, I have already made the decision. Somehow I feel that for PHP7 we have to start with as clean table as possible.

And it's me who will make version and release management, so it's my problem (and for it isn't). Your mileage may vary.

P.S. in fact, the repository is just the same. You can have igbinary6 and igbinary7 remotes locally if you want to. But in GitHub I want them to be separate.

@pierrejoye

This comment has been minimized.

Copy link
Member

pierrejoye commented Dec 17, 2015

Wondering why still as it makes no sense to me. The kernel does not create
new repository, nor php, or any other projects. It only creates confusions
but ok....your call.
On Dec 17, 2015 10:00 PM, "Oleg Grenrus" notifications@github.com wrote:

Sorry, I have already made the decision. Somehow I feel that for PHP7 we
have to start with as clean table as possible.

And it's me who will make version and release management, so it's my
problem (and for it isn't). Your mileage may vary.

P.S. in fact, the repository is just the same. You can have igbinary6 and
igbinary7 remotes locally if you want to. But in GitHub I want them to be
separate.


Reply to this email directly or view it on GitHub
#40 (comment).

@rlerdorf

This comment has been minimized.

Copy link
Author

rlerdorf commented Dec 17, 2015

I don't think this is a big deal. A clean igbinary7 repo is fine with me if it makes it easier to manage for @phadej
The important part is getting a working extension for PHP 7, not the little logistics details like this.

@pierrejoye

This comment has been minimized.

Copy link
Member

pierrejoye commented Dec 17, 2015

Right, progresses are progresses. Except that it is definitely not a little
logistics details, not even a details, rather the opposite :)
On Dec 17, 2015 10:21 PM, "Rasmus Lerdorf" notifications@github.com wrote:

I don't think this is a big deal. A clean igbinary7 repo is fine with me
if it makes it easier to manage for @phadej https://github.com/phadej
The important part is getting a working extension for PHP 7, not the
little logistics details like this.


Reply to this email directly or view it on GitHub
#40 (comment).

@rlerdorf

This comment has been minimized.

Copy link
Author

rlerdorf commented Dec 17, 2015

To the person installing a working igbinary extension for PHP 7 from pecl these details are minor. People don't care how the working extension came to be, just that it did.

@pierrejoye

This comment has been minimized.

Copy link
Member

pierrejoye commented Dec 17, 2015

Exactly one of the pain point.

Different repo, different issues trackers (on github). For php5 the dep
(composer now and pickle) will be igbinary but igbinary7 for 7.x, igbinary8
too? You get the idea.

Distros will most likely call igbinary-php7 anyway tho'.

For me I do not mind it much, I know it and when I will have to submit a
patch again, it is easy to switch repository and will most likely focus on
7 anyway. But it is still a pain point at almost any other level for the
users.
On Dec 17, 2015 10:30 PM, "Rasmus Lerdorf" notifications@github.com wrote:

To the person installing a working igbinary extension for PHP 7 from pecl
these details are minor. People don't care how the working extension came
to be, just that it did.


Reply to this email directly or view it on GitHub
#40 (comment).

@Jan-E

This comment has been minimized.

Copy link

Jan-E commented Dec 17, 2015

You can still cherry-pick independent commits from one repository to another

Do not forget to cherry-pick from the fork of @Sean-Der
https://github.com/Sean-Der/igbinary/tree/php7
It was the repo which scored best at all tests.

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