Refactored building ./configure command-line options and environment variables#1120
Merged
morozov merged 1 commit intophpbrew:masterfrom Dec 24, 2019
morozov:variant-builder-refactoring
Merged
Refactored building ./configure command-line options and environment variables#1120morozov merged 1 commit intophpbrew:masterfrom morozov:variant-builder-refactoring
./configure command-line options and environment variables#1120morozov merged 1 commit intophpbrew:masterfrom
morozov:variant-builder-refactoring
Conversation
…t variables In order to be able to build PHP 7.4 using Homebrew-managed dependencies (\#1085), the variant builder should not only be able to build command-line options for the `./configure` command but also generate the `PKG_CONFIG_PATH` environment variable. Currently it's implemented via storing the `PKG_CONFIG_PATH` components in the build object which then gets passed to the `ConfigureTask`. This pull request refactors this implementation by introducing the `ConfigureParameters` immutable value object that can be used for building command-line options and environment variables for the `./configure` command without storing anything in the `Build` object state or global state. All variant closures now take the a value of `ConfigurationParameters` and return a new version augmented with the parameters representing the variant. In addition to refactoring, the following cleanup and improvements have been done: 1. Changes in the `BuildSettings` class: - All properties have been made `private`. - The absence of a user-provided value for the variant is stored as `null`, not as `true`. This is more semantically correct, corresponds to the API of the variant builder closures and makes their code more straightforward. - The `#hasVariant()` method has been removed since it duplicates `#isEnabledVariant()`. - The `#isDisabledVariant()` method has been removed because it's not used anywhere. - The `#getVariants()` method has been renamed to `#getEnabledVariants()` for consistency with `#getDisabledVariants()`. - The `#getVariant()` method has been removed since it was needed for an edge case that has been reworked to not rely on it. - The `#grepExtraOptionsByPattern()` method has been removed. It violates the SRP: the callers should filter using whatever suitable implementation if needed. - The `#resolveVariants()` no longer returns a value since it's an implementation detail. The method may be removed later as unnecessary from the API perspective. 2. A new implementation of `PrefixFinder`, the `UserProvidedPrefix` class has been added. It helps avoid the repetitive `if ($prefix === null)` conditions in the variant builder closures. 3. The default `./configure` options like `--with-config-file-path` have been moved from `ConfigureTask` to `InstallCommand` in order for the user-provided options to have higher precedence. 4. The `ENV_*` constants, the `$phpEnvironment` property and the `#getIdentifier()` method of the `Build` class have been removed as unused. 5. Changes in the `VariantBuilder` class: - Most of the `public` properties have been made `private`. - The `#checkConflicts()` method has been made `private`. - The `#checkPkgPrefix()` method has been removed as unused. 6. The deprecated `icu` variant has been removed. The `intl` variant will detect the needed build prefix or, otherwise, it can be passed as an extra `--with-icu-dir` option. 7. The recently added warning in the `sodium` variant have been removed since they create unnecessary noise in the test output. Additionally, the `mcrypt`, `opcache` and `phpdbg` version-specific variants don't emit such warnings. As needed, the version awareness should be introduced on the framework level. 8. The `xml` variant no longer ignores the user-provided value. 9. The `xml_all` has been removed since it duplicates `xml`. 10. The `$prefix` / `$val` arguments of the variant builder closures have been consistently renamed to `$value` since they represent the user-provided value for the variant which is not always a prefix. The argument has been made required since it's part of the API and is always passed by the calling code.
Contributor
Author
|
@jasny I decided to remove the warnings in the @markwu please check if the functionality implemented in #1085 still works. I may have accidentally broken something but I cannot test it on macOS. |
Contributor
|
I tested under It should be $ patch --forward --backup -p1 < 2667.patch
patching file ext/openssl/openssl.c
patching file ext/openssl/tests/bug41033.phpt
patching file ext/openssl/tests/bug66501.phpt
patching file ext/openssl/tests/openssl_error_string_basic.phpt
patching file ext/openssl/tests/sni_server.phpt
patching file ext/openssl/xp_ssl.c
patching file ext/phar/util.c
patching file ext/openssl/openssl.cI think it is not related to this refactoring, it's due to merge patch file into script itself. I will try to figure out what's going on. |
Contributor
|
I figure it out, kindly see the comment here #1093 (comment). |
Contributor
Author
|
Thank you. Let me see if it's possible to preserve the original line endings w/o any encoding like base64. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In order to be able to build PHP 7.4 using Homebrew-managed dependencies (#1085), the variant builder should not only be able to build command-line options for the
./configurecommand but also generate thePKG_CONFIG_PATHenvironment variable. Currently, it's implemented via storing thePKG_CONFIG_PATHcomponents in the build object which then gets passed to theConfigureTask.This pull request refactors this implementation by introducing the
ConfigureParametersimmutable value object that can be used for building command-line options and environment variables for the./configurecommand without storing anything in theBuildobject state or global state.All variant closures now take the a value of
ConfigurationParametersand return a new version augmented with the parameters representing the variant.In addition to refactoring, the following cleanup and improvements have been done:
BuildSettingsclass:private.null, not astrue. This is more semantically correct, corresponds to the API of the variant builder closures and makes their code more straightforward.#hasVariant()method has been removed since it duplicates#isEnabledVariant().#isDisabledVariant()method has been removed because it's not used anywhere.#getVariants()method has been renamed to#getEnabledVariants()for consistency with#getDisabledVariants().#getVariant()method has been removed since it was needed for an edge case that has been reworked to not rely on it.#grepExtraOptionsByPattern()method has been removed. It violates the SRP: the callers should filter using whatever suitable implementation if needed.#resolveVariants()no longer returns a value since it's an implementation detail. The method may be removed later as unnecessary from the API perspective.PrefixFinder, theUserProvidedPrefixclass has been added. It helps avoid the repetitiveif ($prefix === null)conditions in the variant builder closures../configureoptions like--with-config-file-pathhave been moved fromConfigureTasktoInstallCommandin order for the user-provided options to have higher precedence.ENV_*constants, the$phpEnvironmentproperty and the#getIdentifier()method of theBuildclass have been removed as unused.VariantBuilderclass:publicproperties have been madeprivate.#checkConflicts()method has been madeprivate.#checkPkgPrefix()method has been removed as unused.icuvariant has been removed. Theintlvariant will detect the needed build prefix or, otherwise, it can be passed as an extra--with-icu-diroption.sodiumvariant have been removed since they create unnecessary noise in the test output. Additionally, themcrypt,opcacheandphpdbgversion-specific variants don't emit such warnings. If needed, the version awareness should be introduced on the framework level.xmlvariant no longer ignores the user-provided value.xml_allhas been removed since it duplicatesxml.$prefix/$valarguments of the variant builder closures have been consistently renamed to$valuesince they represent the user-provided value for the variant which is not always a prefix. The argument has been made required since it's part of the API and is always passed by the calling code.phpbrew variantsexample output is removed from README since it's prone to getting stale and requires additional maintenance.