search

php / web-rmtools

The PHP release management tools
13 stars 22 forks source link

Attempt to fix runkit7 PECL builds on Windows #10

Closed TysonAndre closed 5 years ago

TysonAndre commented 5 years ago

See https://marc.info/?l=pecl-dev&m=155562978906879&w=2

--enable-extname will attempt to build that extension as a static extension, which won't produce a dll that can be used on windows.php.net

cc @cmb69

weltling commented 5 years ago

@TysonAndre while this is an issue you caught, the actual issue is not that. There is already https://pecl.php.net/package/runkit. The build scripts are currently not prepared and are confused by package names different from extension names different from tarball names. We can't push runkit7 into runkit folders, as that won't be linked properly and will cause confusion. The easiest way to fix this is to name the extension properly runkit7. Otherwise, there needs to be anyway a proper fix to the build scripts to handle the aforementioned situation. I'd be glad to support someone who wants to contribute an implementation, as time doesn't permit to work on this.

Thanks.

TysonAndre commented 5 years ago

rmtools is already capable of publishing an extension where the name of the DLL is different from the extension (and the same as a different extension)

The apcu_bc zip files contain php_apc.dll (to provide APCIterator, to make it easy to go from APC to APCu)

[apcu_bc]
;real_name=apc
type=enable
libs[]=
exts[]=apcu
opts[]=--enable-apc=shared
no_conf=1

https://windows.php.net/downloads/pecl/releases/apcu_bc/1.0.5/

cmb69 commented 5 years ago

AIUI, the problem is that the runkit7 DLLs would end up in the runkit download folder. Other than that, it's quite confusing to have php_runkit.dll for runkit7 as well as original runkit. What's the status of original runkit? Will there be an update for PHP7, @zenovich?

weltling commented 5 years ago

@TysonAndre yes, this way it might work, several quirks have been implemented but they don't always guarantee the result (fe pecl_http). What about headers installed? Other nuances are in the game as well, see below.

APC is the case, where we know it's for sure not coming back, APCU was a clean fork that introduced several compat pieces, which is fine. APC was therefore left to PHP < 5.4 so it could be fetched, etc. It is a different story runkit, i'm not sure to have heared any statement it being abandoned, while there's indeed no activity since 2015. Still, pushing a conflicting package is not a very good idea, as the original one can be resurrected at any time. If you're to take over the original runkit, then it needs to be clarified with the original maintainers. If runkit7 is a fork by intention, which is perfectly fine, then it should live it's own life and avoid conflicting with the original package. There's a certain responsibility, you know, to deal with this kind of things properly.

Thanks.

TysonAndre commented 5 years ago
  1. It would be inconvenient for users to migrate from extension=runkit to extension=runkit7 on production, when there is no reason or ability to run two different versions
  2. Renaming the headers is doable unless it interferes with builds. I would appreciate any assistance with that.
  3. It's not technically practical or desirable to run two different forks of runkit at the same time. They have to track all of the manipulations made to method, functions, constants, etc., so that those can be undone when the module is unloaded.
  4. I had offered to cooperate with migrating "runkit" to php7 in 2015. That offer was turned down due the maintainer disagreeing with my fork dropping php 5 support.

  5. The current maintainer of runkit doesn't have the time to add patches to runkit anymore but would consider it with financial compensation - I'm not aware of any compensation agreements being made. Refer to the issue "PHP7 support" in https://github.com/zenovich/runkit/issues

    I doubt that the current maintainer would agree with me taking over the "runkit" pecl.

A year and a half ago I had a lot of free time and started working on support for PHP7 in Runkit. I thought there had to be some people and companies interested in and willing to pay for that. There were some, actually, but $1,000 were the most generous offer from them. For that reason, I postponed the PHP7 support although I had implemented some part of this task.

Now I have a regular job and don't have much time for doing something else. But, of course, if it's possible to fundraise $100,000 for that, I will spend all my free time working on the project.

I haven't tried Kickstarter yet. Do you think it can help indeed?

weltling commented 5 years ago

@sgolemon @zenovich would it be ok to provide the runkit7 package in the form requested?

Thanks.

TysonAndre commented 5 years ago

would it be ok to provide the runkit7 package in the form requested?

More context: This is already provided as runkit.so on Mac and Linux, where pecl install will build from source. This PR just fixes a bug in the Windows build script.

zenovich commented 5 years ago

@weltling I don't see the need for renaming 'runkit7' into 'runkit'? 'runkit7' is a new extension and it should have its own unique name. Probably, its configruation settings and global variables/functions/classes/constants should be renamed as well. If someone doesn't like the name 'runkit7', I can suggest renaming it into 'gunkit', 'drunkit', 'coolkit' or into something else. But the name 'runkit' is occupied.

Btw, is it okay that the pecl installer on Mac/Linux allows to substitude an extension with a different one? Can I do the same with, say, PDO extension?

TysonAndre commented 5 years ago

I'll keep these guidelines in mind if I fork any PECL projects in the future. When I started the fork in 2015, there wasn't any formal policy that I was aware of relating to PECL packages that no longer received updates (except an awareness of apcu_bc publishing apc).

This fork has been creating the artifact runkit.so/runkit.dll (if built from source) since 2015. It'd require unnecessary effort (relative to any benefits) to force users to change php.ini (and probably function names) from runkit to something else - Doing this doesn't benefit end-users or package maintainers.

weltling commented 5 years ago

Thanks for more context, @TysonAndre. Don't you think it would be in first place inconvenient to ignore the point of one of the original maintainers and continue arguing about the naming? Making a clean fork and focusing on the technical side would be a much better approach.

Thanks.

TysonAndre commented 5 years ago

Don't you think it would be in first place inconvenient to ignore the point of one of the original maintainers and continue arguing about the naming? Making a clean fork and focusing on the technical side would be a much better approach.

This discussion should have been made before https://github.com/php/web-rmtools/commit/0c6604b85ef978cfef60ebe4ac9b3f2037a1e5d2 or while it was on the mailing list or when the runkit7 fork was made on github.

"runkit" and "classkit" had many other former maintainers, who haven't weighed in yet.

I've stated my thoughts about the costs vs benefits earlier in this discussion, and would still prefer the original function and extension names.

TysonAndre commented 5 years ago

cc @nikic @rlerdorf @cmb69 This is already available on PECL as https://pecl.php.net/package/runkit7 with the extension name "runkit". This PR is a one line change that fixes windows builds. (hopefully)

  1. Is anyone able to merge the change?
  2. If there are significant disagreements among PECL admins over the choice to publish a "runkit7" PECL when the "runkit" PECL has not received updates since 2015, how would they (or PECL arguments in general) be resolved or voted on.
weltling commented 5 years ago

@TysonAndre several things can go unnoticed, it doesn't mean they should be done :) This question should have been clarified IMO before the package has been even pushed and there should have been a clear statement from the original project. The way it is done to go unnoticed is IMO bad and disrespectful as well as the way you push for that. Imagine yourself in the reverse situation. I can't decide for you what is right and it is not the right place to discuss this. I've reverted this config for now, until this topic is clarified.

Thanks.

nikic commented 5 years ago

Just my 2c: We have lot of problems with unmaintained extensions. Lots of people spend time updating other peoples extensions for new PHP versions and then pushing persistently, often for months, to get the already implemented changes merged. I remember mailparse and zmq being particularly problematic, not even sure if they are compatible by now or not.

I think in a case like this, where the original extension is unmaintained or the maintainer has no interest in merging PHP 7 support, we should support the new fork to the best of our ability, including efforts to make it a drop-in replacement for the original extension in terms of naming.

TysonAndre commented 5 years ago

The way it is done to go unnoticed is IMO bad and disrespectful as well as the way you push for that. Imagine yourself in the reverse situation. I can't decide for you what is right and it is not the right place to discuss this.

In the reverse situation, I would have either:

  1. Passed the extension off to a suitable maintainer if I no longer had the time/resources to maintain the extension.
  2. If (1.) was not possible, I would have clearly communicated that the "official" repo was abandoned and would not receive updates.

I made a reasonable effort to notify pecl-dev, and started my work by attempting to contribute to "runkit". What additional channels would you have suggested?

I can't decide for you what is right and it is not the right place to discuss this. I've reverted this config for now, until this topic is clarified.

What is the right place to discuss this?

zenovich commented 5 years ago

@nikic

I think in a case like this, where the original extension is unmaintained or the maintainer has no interest in merging PHP 7 support

Where did these two speculations come from?

weltling commented 5 years ago

My concern is that @zenovich was not really heared, he told he worked on 7 port and counted on getting some donations? There's sure a feasible way for runkit7, fe jsond, apcu and some other was from the same approach. Naming a DSO is almost a zero effort. Whatsoever else, the adoption is important - but at any cost, disregarding some other dev for no reason? Again, i'm not the person to solve this and yes still no port in two years is not good. My initial question was to figure out an usual packaging mistake, not to start this awkward conversation. But how it sounds, a proper DSO name at least would be an appropriate solution to this completely non technical issue.

Thanks.

nikic commented 5 years ago

@weltling I agree that it would have been better to use a different DSO name in the first place (while keeping API names) for infrastructure simplicity, but it seems like a bad idea to rename it now, as this will mean that a pecl update is going to break people's setups.

cmb69 commented 5 years ago

Please also consider the documentation in the PHP manual. It is not possible to document functions with the same name twice, so if the function names of runkit and runkit7 are the same, they would have to be documented on the same pages. That might cause issues, if either extension introduces changes. In my opinion, the overall best solution would be if @zenovich and @TysonAndre could somehow manage to merge both extensions. :)

weltling commented 5 years ago

@nikic it is still alpha and has been downloaded ~200 times. In alpha, and on this short term, it doesn't seem a huge issue. And on the other level - it seems a project collision where I'd carefully avoid taking sides. It might be however more or less predictable, that at some point runkit7 would reach the critical mass anyway, if runkit stays 7 incompatible and doesn't accept contributions. In case things change, pecl install would be an issue for both.

@cmb69 yep, merging would be the best strategy. But also it were absurd to require the symbol naming. Any extension can manipulate the function/class table, by coincident even runkit can :)

@zenovich yes, technically it is possible to produce any DSO name, or even multiple DSOs with arbitrary names, etc. It's a useful feature, actually.

Thanks.

TysonAndre commented 5 years ago

the overall best solution would be if zenovich and TysonAndre could somehow manage to merge both extensions.

A year and a half ago I had a lot of free time and started working on support for PHP7 in Runkit. I thought there had to be some people and companies interested in and willing to pay for that. There were some, actually, but $1,000 were the most generous offer from them. For that reason, I postponed the PHP7 support although I had implemented some part of this task.

Now I have a regular job and don't have much time for doing something else. But, of course, if it's possible to fundraise $100,000 for that, I will spend all my free time working on the project.

I haven't tried Kickstarter yet. Do you think it can help indeed?

Merging management of both repos seems unlikely to happen given past discussions. Also, I don't want monetization of any php7 or future php8 projects I'm heavily involved in.

Also, runkit7 is a drop-in fork (supporting the subset of functionality that was feasible to port); All of the work on the technical side has been done.

I would consider the best solution if he voluntarily stepped down as the maintainer of the "runkit" PECL and transferred ownership. If the conditions for him to resume this or a similar project are met after 4 years, publishing it under a different name with different function names would cause the least confusion on docs.php.net.

I think in a case like this, where the original extension is unmaintained or the maintainer has no interest in merging PHP 7 support

Where did these two speculations come from?

The "funkit" approach of a clean rename with compatibility seems to be the most practical approach, and has the mentioned benefits of significantly clearer documentation on php.net (e.g. easier to tell which functions are supported) for end users, at the cost of one-time operational work to upgrade.

Funkit proposal

@weltling - Is this PECL proposal acceptable?

cc @remicollet - Would this cause any problems for software packaging or end users using your repo?

  1. Create a "funkit" extension (focusing on function manipulation), which has funkit_superglobals(), funkit_method_rename(), etc. Continue using the runkit.superglobals and runkit.internal_method_override config setting in PHP.ini to minimize the amount of work that needs to be done.

  2. Change the "runkit7" PECL into one that's similar to "apcu_bc" in the next major release: It provides function aliases of runkit_method_rename -> funkit_method_rename() and constant aliases, and depend on "funkit".

    If new "funkit" functions/methods were created, aliases won't be added.

    This may require additional work and testing on web-rmtools to set up the build.

    This would also require these patches to fix the PECL builds on Windows for runkit7's php_runkit.dll to be reapplied

    end users would have to install both "funkit" and "runkit7" (runkit.so/php_runkit.dll) and change

  3. Publish funkit 3.0.0 and runkit7 3.0.0
krakjoe commented 5 years ago

I've had some time to discuss this and rethink.

The things being disputed are:

@TysonAndre we can't hand over the name "runkit" for the package, that belongs to @zenovich, the runkit7 dso must be named appropriately (it can't be named runkit.so).

@zenovich we can't stop @TysonAndre providing the same API as runkit with runkit7, and nor should we.

It's common place for projects with different names to provide the same API to end users, it happens all the time. It can and should happen here.

The problem remains that if runkit7 provides the same API as runkit, we have no place to document the differences. I don't know what solution there is for that, but given the probably low number of runkit users it may not be that much of an issue.

We should support Tyson moving forward, renaming the DSO should be enough to get the build working, and means only a configuration update for users, which is about the least friction there could be in these circumstances.

nikic commented 5 years ago

I agree with the course of action outlined in @krakjoe's last comment. The DSO should be renamed to match the extension name, and everything else can stay. If there is a need to introduce API discrepancies in the future, those can be done under the runkit7 prefix (similar to how apcu functions sometimes differ from the apc_ BC functions).

TysonAndre commented 5 years ago

the runkit7 dso must be named appropriately (it can't be named runkit.so).

I'm willing to do that (despite the drawbacks mentioned in https://github.com/php/web-rmtools/pull/10#issuecomment-492322934 )

If I understand correctly, I'd have to do the following in the next major release:

  1. Change in package.xml (etc.) to runkit7
  2. Change --enable-runkit to --enable-runkit7 (so that Windows builds will work). Change --enable-runkit-modify to --enable-runkit7-modify (and so on).
  3. Change php_runkit.h to php_runkit7.h (and any other installed headers)
  4. Change config.m4 and config.w32 to define the extension runkit7 instead of runkit, and to define RUNKIT7 equivalents of gcc macros (and install the new headers)
  5. Communicate php.ini and build flag changes in the release notes. (perhaps emit a warning similar to what xdebug does to recommend using zend_extension=xdebug.so instead of extension=xdebug.so, but say that extension=runkit.so should be removed and replaced with runkit7.so)
  6. Publish 3.0 release candidate and verify (e.g. check windows.php.net builds)
  7. Publish 3.0

https://github.com/runkit7/runkit7/pull/196 was introduced to add the runkit7_*() and RUNKIT7_* aliases of global functions and constants, so that creating unambiguous documentation would be easier.

Those will be left in for several months, to make updating easier for library authors and end users - a composer package can be published later as a polyfill once the old constants/functions are removed.

weltling commented 5 years ago

@TysonAndre thanks for paying attention to this. Properly naming the DSO/DLL, as suggested before, would be the line of the least resistance not causing you a lot of extra work and otherwise fixing the collision with the original project. Even, for the doc, another option would be to put the symbols into a namespace, then the userspace fix would be as easy as import the symbols from the namespace. In any case, please keep the appreciated good work!

I'm also on the same page as Joe, and that's why the fact the naming was intentional and not a usual mistake stood out to me. It would strike at some other place, if not here, anyway. It is a very sensitive situaiton where the scale can be easily overturn into a wrong side. The situation to have no runkit port in this late stage of 7.x is not nice, but on the other hand punishing someone for trying to fund an OSS project or not having time to contribute is barely a right thing. There are quite some other aspects on this, especially also what potential new contributors see, so we should really think as a community and find appropriate solutions.

Thanks.

TysonAndre commented 5 years ago

It's published as extension=runkit7.so/php_runkit7.dll (changing all configure options from --enable-runkit* to --enable-runkit7*) as https://windows.php.net/downloads/pecl/releases/runkit7/3.0.0a1/ . If there's any feedback on the configuration scripts or any bug reports, let me know - I plan to release 3.0.0 a week from now.

The windows builds aren't visible on pecl.php.net for 3.0.0a1 https://pecl.php.net/package/runkit7 - I assume this is a spurious issue.

I agree with the course of action outlined in @krakjoe's last comment. The DSO should be renamed to match the extension name, and everything else can stay. If there is a need to introduce API discrepancies in the future, those can be done under the runkit7 prefix (similar to how apcu functions sometimes differ from the apc_ BC functions).

The renaming of the DSO is done - there's already the discrepancy of supporting strict_types=1 and return types when adding or redefining methods/functions. (predating adding this to PECL)

php-pulls commented 5 years ago

Comment on behalf of cmb at php.net:

Now all Windows builds are available, so I'm closing this ticket. Thanks for resolving the naming issues @TysonAndre!

weltling commented 5 years ago

@TysonAndre builds not appearing on the pecl site at once is ok, please give it a couple of hours after the release for the pecl site to pick up. If they don't appear for some longer than that, it might be really an issue.

Thanks.