Skip to content

Fix phpize build of the opcache extension (jit/zend_jit.lo not found) #6589

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

Closed
wants to merge 3 commits into from

Conversation

samm-git
Copy link
Contributor

@samm-git samm-git commented Jan 8, 2021

I found that jit is not build correctly on my FreeBSD machine using phpize command in the extension directory.

Reason is that in the Makefile i have referenced jit/zend_jit.lo as dependency, but target was mentioned as ./jit/zend_jit.lo. This fix adds additional target w/o path which is used on local builds with phpize

@samm-git samm-git changed the title Remove builddir prefix for the jit/zend_jit.lo WIP: Remove builddir prefix for the jit/zend_jit.lo Jan 8, 2021
@samm-git samm-git changed the title WIP: Remove builddir prefix for the jit/zend_jit.lo Remove builddir prefix for the jit/zend_jit.lo Jan 9, 2021
@samm-git samm-git changed the title Remove builddir prefix for the jit/zend_jit.lo Fix phpize build of the opcache extension (jit/zend_jit.lo not found) Jan 9, 2021
@afilina
Copy link
Contributor

afilina commented Jan 14, 2021

I feel we need a test to validate the bug and the fix. Perhaps if the build fails on certain systems, we'd need a job in the pipeline to build on that systems, as to catch those bugs. Can the maintainers chime in on this?

@KalleZ KalleZ requested a review from dstogov January 14, 2021 20:59
@samm-git
Copy link
Contributor Author

@afilina i think it should be extra-easy ti reproduce, just by bulding this extension standalone and not as part of php build.

@nikic
Copy link
Member

nikic commented Jan 18, 2021

I just ran:

cd ext/opcache
~/php-install/bin/phpize
./configure --with-php-config=$HOME/php-install/bin/php-config
make -j32

And this worked fine for me.

@nikic
Copy link
Member

nikic commented Jan 18, 2021

Could this be due to a FreeBSD-specific difference in make behavior? I see in the generated makefile that there is a indeed an inconsistency between jit/zend_jit.lo and ./jit/zend_jit.lo being mentioned in different places, but this seems to work fine on Linux (GNU Make 4.2.1).

If that's the case, I think it might be better to adjust PHP_ADD_MAKEFILE_FRAGMENT to replace ($builddir)/ with empty string in the case where $builddir is ., as this wouldn't be the only place where this issue can occur.

@samm-git
Copy link
Contributor Author

Let me try to reproduce this on Linux....

@samm-git
Copy link
Contributor Author

It seems that you right. I was not able to reproduce that on Linux (ubuntu)

  1. Unpack https://www.php.net/distributions/php-8.0.1.tar.gz to the local filesystem
  2. ./configure --disable all && make && make install && cd ..
  3. rm -rf php-8.0.1 && tar -xvzf php-8.0.1.tar.gz
  4. cd php-8.0.1/ext/opcache/ && /usr/local/bin/phpize && ./configure && make

And it works! This ubuntu using GNU Make 4.2.1. I will try to find whats causing here FreeBSD to fail

@samm-git
Copy link
Contributor Author

@nikic this is true, error happens only with BSD make and does not happens with GNU make. We can either fix it as "non-fix" and suggest to use GNU make here, or just to apply this patch if you dont see any downsides from it.

@samm-git
Copy link
Contributor Author

Yes, BSD make want exact match of the target name:

This is sample Makefile:

all: test

./test:
	@echo "hello"

And test results:

root@12_2amd64-default:~ # make
make: don't know how to make test. Stop

make: stopped in /root

root@12_2amd64-default:~ # gmake
hello

@nikic
Copy link
Member

nikic commented Jan 25, 2021

I'm okay with just applying this patch. It's a bit of a hack, but I think we understand now why it's needed and it seems pretty harmless.

@php-pulls php-pulls closed this in 527bcb1 Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants