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

Remove major version from apache module #3769

Closed
wants to merge 1 commit into from
Closed

Conversation

nikic
Copy link
Member

@nikic nikic commented Jan 28, 2019

Having the major version in the SO/module name doesn't make a lot of sense to me, as our ABI stability guarantees apply to minor versions. Unless we want to include both the major and minor version here, we should just drop it entirely.

We don't include major/minor version in other artifacts either and leave it to distros to rename binaries to their liking (php-7.2 etc).

The only other place where we include the major version number in artifacts are the primary Windows DLLs (php7ts.dll). Maybe @weltling can comment on why that is done?

@KalleZ
Copy link
Member

KalleZ commented Jan 28, 2019

I don't think there is specific reasoning behind doing this on Windows, I can't think of any load order collision (as the current directory of the executable or DLL (in case of Apache) is used to search for missing ones before resolving to the PATH).

It is however a slight web to untangle the code in win32/

@krakjoe krakjoe added the PHP 8 label Jan 29, 2019
@nikic nikic removed the PHP 8 label Jan 29, 2019
@nikic nikic added this to the PHP 8.0 milestone Jan 29, 2019
@petk
Copy link
Member

petk commented Jan 30, 2019

That's good I think. Not to mention the option of prefix and suffix on *nix builds which should take care of having multiple versions at the same time. Maybe we can also implement this more properly and user has most of the options to change end program name (in all files that matter for this):
https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Transforming-Names.html

@petk petk added the Feature label Jan 30, 2019
@nikic
Copy link
Member Author

nikic commented Feb 5, 2019

As nobody objected here / knew a reason why we need this, I went ahead and merged this as 6e3600f. If it turns out this was necessary for whatever reason, let's revert it.

@nikic nikic closed this Feb 5, 2019
@petk
Copy link
Member

petk commented Feb 5, 2019

Thanks @nikic The only reason this was used was that there were multiple versions of the module possible on one server installation (5 and 7). However, most distributions use also additional more detailed patches to make module versions possible. With autoconf options we can make it more proper. I'll check and sync this with some other opened pull requests...

@weltling
Copy link
Contributor

weltling commented Feb 6, 2019

@nikic oh, i was just too slow in response, to busy with other work. Anyway, about the DLL names - it seems to have been done somewhere in early PHP 5, so i guess there was issues as @KalleZ mentioned. In general, strange naming of DLLs is usually attribute to the DLL hell prevention. We could indeed work on simplifying this, but it'd require a control layer for the checks which linker has been actually used. That layer will have to be integrated into the SAPIs, so they know which php[ts].dll version and linker are supposed to be compatible. The shared extensions have a partial check for the PHP version, but not for the linker. This might improve the situation a lot, where multiple PHP versions might be present on the systems and interfering on the path. So this might be done alltogether with removing the remains of versions in the files. I note this for the upcoming tasks.

Thanks.

cmb69 added a commit to cmb69/php-src that referenced this pull request Feb 17, 2025
While it may make some sense to have the PHP major version in the names
of the binaries[1], there is no reason to have it in the names of the
source files.

[1] <php#3769>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants