Refactor binary builder

[TisVictress]

Remove binary builder code from compilation

[sophiewigmore]

@TisVictress whats the reason for this PR being a draft still?
Some other questions:

• Have you been able to verify that the output tarballs look similar to what we currently output?
• Have you been able to test compilation for bionic/jammy and both version lines?
• Do the compiled dependencies work in the buildpack?

[TisVictress]

@sophiewigmore I didn't get a chance to add some comments but I split the main file out into internal to make it easier to read. The code still needs to be refactored a bit more to make it more simpler (ie running commands). It now uses the paketo full builders.
The tarballs are not identical to the current output but they are similar but I didn't get a chance to test the latest changes in the buildpack.

The InstallerFactory has methods that match the names of the PHP extensions so the utils can use golang's relect pkg to create installers for each extension. I split the extensions out into separates files as well, one for native modules and the other for Pecl extensions.

Compilation works for jammy and bionic.

[sophiewigmore]

The code still needs to be refactored a bit more to make it more simpler (ie running commands).

Are you specifically looking for feedback on that? Or is it in progress?

The tarballs are not identical to the current output but they are similar but I didn't get a chance to test the latest changes in the buildpack.

This is pretty important so we can verify that we haven't changed any behaviour with the new dependencies.

[sophiewigmore]

Same comment as on the bionic Dockerfile - If we're compiling on the stack image we shouldn't need to install all of these things because a good number of them should already be there. There may be a few we still need to install

[sophiewigmore]

@TisVictress this comment still stands.

[sophiewigmore]

This comment still stands :/ a lot of these packages are on the Full jammy build image.

[sophiewigmore]

The InstallerFactory has methods that match the names of the PHP extensions so the utils can use golang's relect pkg to create installers for each extension. I split the extensions out into separates files as well, one for native modules and the other for Pecl extensions.

In general the new split has made things clearer, well done.
You have the most context on how the code comes together so I'll defer to you for an answer, but it still feels like there's a lot of duplication. For example, in internal/extension_installer.go, the Pecl extensions all look similar for their functions except Install.

type Redis struct {
	peclExt PeclExtension
}

func (r Redis) DepName() string {
	return r.peclExt.DepName()
}

func (r Redis) DepVersion() string {
	return r.peclExt.DepVersion()
}

func (r Redis) DepSha() string {
	return r.peclExt.DepSha()
}

func (r Redis) URL() string {
	return r.peclExt.URL()
}

func (r Redis) WorkingDir() string {
	return r.peclExt.WorkingDir()
}

Would it be possible to achieve some kind of method override pattern, were PeclExtension type is anonymous inside of each extension type like:

type Redis struct {
	PeclExtension
}

and then, we can use the common PeclExtension methods like URL() and WorkingDir(), but override Install() and any others that are different? I think we could use that same idea for the various "types" of extensions you have here. What do you think?

[sophiewigmore]

small nit - I noticed that the dep.tgz.checksum file that gets output doesn't include the algorithm in the file content

[sophiewigmore]

Just compiled 8.0.27 (bionic) and 8.1.15 (bionic and jammy) with the code here and put them into the buildpack. The integration tests here fail with:

        [builder] failed to evaluate symlink /layers/paketo-buildpacks_php-dist/php/lib/libgpg-error.so: lstat /layers/paketo-buildpacks_php-dis
t/php/lib/lib: no such file or directory
            [builder] ERROR: failed to build: exit status 1
            ERROR: failed to build: executing lifecycle. This may be the result of using an untrusted builder: failed with status code: 51

during the step during the build phase where we try to install (and untar) the dependency.

[TisVictress]

What do you think?

@sophiewigmore Each extension type is an Installer and needs to implement the Installer interface. I don't think this can be done with anonymous types because the InstallerFactory expects each type to have a specific set of methods.

[TisVictress]

Are you specifically looking for feedback on that? Or is it in progress?

Both

[sophiewigmore]

@TisVictress have you had a chance to test out these changes in the buildpack? That's how I discovered the failures last time

[TisVictress]

@sophiewigmore This looks like a bad merge. I have some files in here that should be deleted. I'll update the PR...sorry about that

[TisVictress]

@TisVictress have you had a chance to test out these changes in the buildpack? That's how I discovered the failures last time

@sophiewigmore Yes...I used the integration tests and they were green.

[robdimsdale]

In general I think this looks great. Thanks for all the hard work @TisVictress - I appreciate you sticking with it for so long!

My review has essentially only been around writing Golang / using packit. I don't have enough context on the php extensions to have an opinion on any of those.

I have a couple of minor suggestions about improved error handling and resiliency (inline). But I don't think any of them should block the merging of this PR.

[sophiewigmore]

I am still wondering why we need to install this entire list of packages if were building on the stack

[TisVictress]

Oh yes sorry @sophiewigmore I'll see what can be removed

[sophiewigmore]

there may be packages we need here that aren't in the stack image though so might need to do a cross-check against the build image receipt

[sophiewigmore]

@TisVictress this comment still stands, a large number of these packages are in the Bionic stack build image. :/

[sophiewigmore]

Looking at the list of extensions here, it seems like some extensions that were in the old extensions-manifests/extensions-8.2.yml file are missing, like pdo_oci and oci8, and possibly others. I didn't check 8.0 and 8.1 but we need to make sure we're not missing extensions

[TisVictress]

@sophiewigmore I removed them because it doesn't seem that they are actually getting installed. For example, there is a check for oci8 that seems to always return false for the currently supported versions (https://github.com/cloudfoundry/binary-builder/blob/main/recipe/php_meal.rb#L202). And some extensions were causing make test to fail, because the test wasn't expecting to see these extensions pop up. I'll rerun the test and post the failing extensions here. If you think I should add them back, I can update the current tests to include them.

[sophiewigmore]

@TisVictress that's strange... hmmm. It could be worthwhile just double checking if the versions of PHP in the buildpack.toml now have those extensions or not. If not then we're probably OK to leave them out, but I'm worried to stop supporting them if they do somehow end up available in the versions of PHP we currently ship, you know?

[sophiewigmore]

Any update on this?

[sophiewigmore]

We're still installing a whole bunch of packages that are on the stack image (starting on line 10). Can we please pare this down?

[sophiewigmore]

@TisVictress btw I was able to compile 8.1.21 on bionic, but 8.2.8 takes a really long time and errors out on my machine. Are you able to do it on your machine?

[TisVictress]

@TisVictress btw I was able to compile 8.1.21 on bionic, but 8.2.8 takes a really long time and errors out on my machine. Are you able to do it on your machine?

@sophiewigmore No I haven't had any issues. Would you mind trying again?

[sophiewigmore]

@TisVictress what should we do with this PR?