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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

php.extensions.snuffleupagus: init at 0.7.0 #120699

Merged

Conversation

NorfairKing
Copy link
Contributor

@NorfairKing NorfairKing commented Apr 26, 2021

Motivation for this change

#120163

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"

I ran this, but have no idea what it does or what I should see. It succeeded .. 馃し

  • Tested execution of all binary files (usually in ./result/bin/):
result/bin/php -m | grep snuffleupagus
snuffleupagus
  • 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.

@NorfairKing
Copy link
Contributor Author

CC @karantan :D

@NorfairKing
Copy link
Contributor Author

I marked the discussions as resolved while I'm trying to push, but "git push" seems to take very long, appologies.

@NorfairKing NorfairKing force-pushed the package-php-snuffleupagus branch 2 times, most recently from cfb89b1 to 5db443b Compare April 26, 2021 08:36
@etu
Copy link
Contributor

etu commented Apr 26, 2021

The commit message could be worded php.extensions.snuffleupagus: init at 0.7.0 since that's the name of the attribute of only this extensions 馃檪

@NorfairKing NorfairKing changed the title php snuffleupagus: init at version 0.7.0 php.extensions.snuffleupagus: init at 0.7.0 Apr 26, 2021
@r-rmcgibbo
Copy link

Result of nixpkgs-review pr 120699 at 5db443bd run on x86_64-linux 1

2 packages failed to build:
1 package built successfully:
  • php74Extensions.snuffleupagus

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement.

@NorfairKing
Copy link
Contributor Author

@etu I have no idea what these build failures are, or how to reproduce them. Do you know how I could reproduce them locally? Then I can try to fix things.
To be honest I have never packaged a PHP module before so I welcome all the help I can get :)

@ofborg ofborg bot requested a review from etu April 26, 2021 08:45
@etu
Copy link
Contributor

etu commented Apr 26, 2021

@etu I have no idea what these build failures are, or how to reproduce them. Do you know how I could reproduce them locally? Then I can try to fix things.

This is how to build them for each PHP version:

nix-build -A php73.extensions.snuffleupagus -A php74.extensions.snuffleupagus -A php80.extensions.snuffleupagus

The 7.4 version seems to work, but we have different failures on 8.0 and 7.3 馃檪

@NorfairKing
Copy link
Contributor Author

I was able to fix the older php versions but the php80 version still fails here:
https://github.com/jvoisin/snuffleupagus/blob/654552c14ba8c98a244f82f9b8f1225a68526efb/src/php_snuffleupagus.h#L59
I'm not sure what to do about it..

@NorfairKing
Copy link
Contributor Author

@SuperSandro2000 I'm fine with your suggestions. Could you please add them to the commit and merge?

@aanderse
Copy link
Member

@NorfairKing please squash.

@NorfairKing
Copy link
Contributor Author

@aanderse squashed.

description = "Security module for php7 and php8 - Killing bugclasses and virtual-patching the rest!";
license = licenses.lgpl3Only;
homepage = "https://github.com/jvoisin/snuffleupagus";
maintainers = teams.php.members;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
maintainers = teams.php.members;
maintainers = teams.php.members ++ maintainers.niteoweb;

or whatever is your name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer not to maintain this. If anyone, it should be niteoweb.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you want to use it? Well we don't need to add you but putting the maintenance burden on the php team is not nice and it might just break over time because no one fixes it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SuperSandro2000 Yes that's a fair point, is there a way to add an organisation as a maintainer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like a company? We have several companies which are listed as teams of maintainers, if that is what you mean. Very welcome, if so.

Is that what you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aanderse. I've added the head of the company: @zupo as the maintainer because he was already in the maintainers list.
It's niteoweb that will be using this, I just packaged snuffleupagus for them but won't be using it personally.

@ofborg ofborg bot requested a review from zupo May 22, 2021 09:43
@SuperSandro2000
Copy link
Member

php80Extensions.snuffleupagus fails to build with

       > In file included from /build/source/src/snuffleupagus.c:7:
       > /build/source/src/php_snuffleupagus.h:49:2: error: #error Snuffleupagus requires PHP7+ with PCRE support
       >    49 | #error Snuffleupagus requires PHP7+ with PCRE support
       >       |  ^~~~~
       > make: *** [Makefile:208: snuffleupagus.lo] Error 1
       For full logs, run 'nix log /nix/store/c430sjhxg5fykyrnzp3nh7a4n3z2670h-php-snuffleupagus-0.7.0.drv'.

@NorfairKing
Copy link
Contributor Author

@SuperSandro2000 I have no idea what this error means. php80Extensions Is PHP 8 right? So that's PHP7+ and there is the pcre' dependency, isn't that PCRE support?

@SuperSandro2000
Copy link
Member

@SuperSandro2000 I have no idea what this error means. php80Extensions Is PHP 8 right? So that's PHP7+ and there is the pcre' dependency, isn't that PCRE support?

No idea either. Upstream does not mention php 8.0. I think we should just set:

meta.broken = lib.versionAtLeast php.version "8";

@talyz
Copy link
Contributor

talyz commented May 29, 2021

The PHP8 issue is fixed upstream, but not in a release yet: jvoisin/snuffleupagus@3c528d9. Adding that commit as a patch as follows solves the issue.

patches = [
  (fetchpatch {
    url = "https://github.com/jvoisin/snuffleupagus/commit/3c528d9d03cec872382a6f400b5701a8fbfd59b4.patch";
    sha256 = "0lnj4xcl867f477mha697d1py1nwxhl18dvvg40qgflpdbywlzns";
    stripLen = 1;
  })
];

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 120699 run on x86_64-linux 1

3 packages built:
  • php73Extensions.snuffleupagus
  • php74Extensions.snuffleupagus
  • php80Extensions.snuffleupagus

@SuperSandro2000 SuperSandro2000 merged commit 3850fdb into NixOS:master Jun 1, 2021
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

6 participants