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

Jump to bottom

php: replace substituteInPlace to patch #90339

Closed
wants to merge 2 commits into from
Closed

php: replace substituteInPlace to patch #90339

wants to merge 2 commits into from

Conversation

Izorkin
Copy link
Contributor

@Izorkin Izorkin commented Jun 14, 2020

Motivation for this change

Remove In the build log lot of warning mesages:

...
substituteStream(): WARNING: pattern '@CONFIGURE_OPTIONS@' doesn't match anything in file 'main/build-defs.h.in'
substituteStream(): WARNING: pattern '@PHP_LDFLAGS@' doesn't match anything in file 'main/build-defs.h.in'
substituteStream(): WARNING: pattern '@CONFIGURE_COMMAND@' doesn't match anything in file 'scripts/php-config.in'
substituteStream(): WARNING: pattern 'test -x "$PKG_CONFIG"' doesn't match anything in file './Zend/acinclude.m4'
substituteStream(): WARNING: pattern 'test -x "$PKG_CONFIG"' doesn't match anything in file './Zend/Zend.m4'
substituteStream(): WARNING: pattern 'test -x "$PKG_CONFIG"' doesn't match anything in file './ext/libxml/config0.m4'
substituteStream(): WARNING: pattern 'test -x "$PKG_CONFIG"' doesn't match anything in file './ext/dba/config.m4'
substituteStream(): WARNING: pattern 'test -x "$PKG_CONFIG"' doesn't match anything in file './ext/xml/config.m4'
substituteStream(): WARNING: pattern 'test -x "$PKG_CONFIG"' doesn't match anything in file './ext/tidy/config.m4'
substituteStream(): WARNING: pattern 'test -x "$PKG_CONFIG"' doesn't match anything in file './ext/date/config0.m4'
substituteStream(): WARNING: pattern 'test -x "$PKG_CONFIG"' doesn't match anything in file './ext/json/config.m4'
substituteStream(): WARNING: pattern 'test -x "$PKG_CONFIG"' doesn't match anything in file './ext/pdo_sqlite/config.m4'
substituteStream(): WARNING: pattern 'test -x "$PKG_CONFIG"' doesn't match anything in file './ext/xmlrpc/libxmlrpc/acinclude.m4'
substituteStream(): WARNING: pattern 'test -x "$PKG_CONFIG"' doesn't match anything in file './ext/xmlrpc/libxmlrpc/xmlrpc.m4'
...

and cleanup preConfigure phase.

cc @NixOS/php

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@Izorkin Izorkin changed the title php: replace ubstituteInPlace to patch php: replace substituteInPlace to patch Jun 14, 2020
@ofborg ofborg bot requested review from globin, Ma27, talyz, aanderse and etu June 14, 2020 13:01
@Izorkin
Copy link
Contributor Author

Izorkin commented Jun 25, 2020

cc @NixOS/php

@aanderse
Copy link
Member

One issue I have with this change is that your patch could potentially miss something if the patched files change in the future, though maybe php simply wouldn't compile in that case?

Currently there is no error, just warning. I think the current heavy handed substitute approach is better as it requires no manual maintaining... does anyone else have an opinion on that?

@etu
Copy link
Contributor

etu commented Jun 26, 2020

I agree with you @aanderse.

If the code change in the future one has to update the patch etc. Then it would become harder to do overrides to get other versions where the current substitutes just works.

I find these warnings to be harmless and as long as it builds it's fine with these warnings. So doing this is just asking for more issues than it's worth.

@Izorkin
Copy link
Contributor Author

Izorkin commented Jun 28, 2020

I don't think that there are going to be any big changes in build process in versions 7.2.x, 7.3.x and 7.4.x.
In php-8.x we also only need to change #define CONFIGURE_COMMAND in main/build-defs.h.in, and ldflags and configure_options in scripts/php-config.in. We don't need to patch pkgconfig in php-8.x at all.
To sum it up, it's not necessary to change anything in the patches in the future, or it might be needed very seldom.

@Ma27 Ma27 removed their request for review August 5, 2020 20:25
@stale
Copy link

stale bot commented Apr 18, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 18, 2021
@etu
Copy link
Contributor

etu commented Aug 1, 2022

I'm thinking that we won't merge this, if someone disagree, make a comment here.

@etu etu closed this Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@globin globin Awaiting requested review from globin globin is a code owner

@talyz talyz Awaiting requested review from talyz talyz is a code owner

@aanderse aanderse Awaiting requested review from aanderse aanderse is a code owner

@etu etu Awaiting requested review from etu

Assignees
No one assigned
Labels
2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 10.rebuild-darwin: 11-100 10.rebuild-linux: 101-500
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Izorkin @aanderse @etu @ryantm