-
Notifications
You must be signed in to change notification settings - Fork 9
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
Restructure rfc #267
Restructure rfc #267
Conversation
b5c6cb7
to
6c44e78
Compare
rfcs/0001-restructure.md
Outdated
|
||
## Environment Variables | ||
|
||
* `php-dist` buildpack must expose the location of the `php.ini` file through |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we can drop this env variable. If we properly install PHP, the cli should be on the path. You'll also get php-config
which is an exec you can use to locate things like the INI file, extensions dir, etc.. so I don't think there's a reason for the buildpacks to create a non-standard way of finding similar info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the ini
file will be dropped on to the php layer by the dependency. How would the php-config
binary or the php-installation know where this file is? Wouldn't it be in a custom named directory in the layer directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, the buildpacks will assume that PHP is installed and available on the path (which php-dist-cnb does). Part of what's installed is php-config
, which you can use to query information like this. You can shell out and run php-config
and ask about the locations of these files.
The only env variables we should set are the standard ones that tell PHP where to find the INI files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We basically just need to delete this line item. The php-dist cnb doesn't need to do this because other cnb's can use php-config
to locate the php.ini file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmikusa-pivotal In the current design, downstream buildpacks do not require php
at build
. For them to use php-config
, php has to be required at build
. I can make them require php
at build as well so they can call php-config
and other binaries that come with php. Does that sound good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I'm tempted to say yes, let's make it available at build so other cnb's can use php-config. Do you see any downside to that? Is there a performance penalty for making it required at build by more cnbs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe there's a penalty. Updated
Signed-off-by: Arjun Sreedharan <asreedharan@vmware.com>
775fcd1
to
88b1db5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Readable