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

Refactored building ./configure command-line options and environment variables #1120

Merged
merged 1 commit into from
Dec 24, 2019
Merged

Refactored building ./configure command-line options and environment variables #1120

merged 1 commit into from
Dec 24, 2019

Conversation

morozov
Copy link
Contributor

@morozov morozov commented Dec 23, 2019

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 warnings 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. If 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.
  11. The phpbrew variants example output is removed from README since it's prone to getting stale and requires additional maintenance.

…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.
@morozov
Copy link
Contributor Author

morozov commented Dec 23, 2019

@jasny I decided to remove the warnings in the sodium variant that we discussed in #1064 (comment). Please see more details in the description.

@markwu please check if the functionality implemented in #1085 still works. I may have accidentally broken something but I cannot test it on macOS.

@morozov morozov merged commit d5f3b4c into phpbrew:master Dec 24, 2019
@morozov morozov deleted the variant-builder-refactoring branch December 24, 2019 01:54
@markwu
Copy link
Contributor

markwu commented Dec 24, 2019

I tested under macOS 10.14.6 with Xcode 11.0 and CLT 11.0, everything works as expected. Only the following error messages:

Checking patch for php5.6 with openssl 1.1.x patch.
---> Applying patch...
patching file ext/openssl/openssl.c
patching file ext/openssl/tests/bug41033.phpt
patching file ext/openssl/tests/bug66501.phpt
Hunk #1 FAILED at 16.
1 out of 1 hunk FAILED -- saving rejects to file ext/openssl/tests/bug66501.phpt.rej
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.c
patch unexpectedly ends in middle of line
Hunk #1 succeeded at 651 with fuzz 1.

Patch failed
0 changes patched.

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.c

I 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.

@markwu
Copy link
Contributor

markwu commented Dec 24, 2019

I figure it out, kindly see the comment here #1093 (comment).

@morozov
Copy link
Contributor Author

morozov commented Dec 24, 2019

Thank you. Let me see if it's possible to preserve the original line endings w/o any encoding like base64.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants