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.

Sign up for GitHub

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

phpunit: init at 10.0.11 #217207

Merged
merged 1 commit into from
Feb 22, 2023
Merged

phpunit: init at 10.0.11 #217207

merged 1 commit into from
Feb 22, 2023

Conversation

onny
Copy link
Contributor

@onny onny commented Feb 19, 2023 •
edited
Loading

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@onny onny marked this pull request as ready for review February 19, 2023 19:33
@ofborg ofborg bot requested a review from drupol February 19, 2023 20:28
drupol
drupol requested changes Feb 19, 2023
View reviewed changes
Copy link
Contributor

@drupol drupol left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hello,

Thanks for your contribution!

I added a request change about moving phpunit in pkgs/development/tools/misc/ instead. Just like in #212296

pkgs/top-level/php-packages.nix Outdated Show resolved Hide resolved
@Ma27
Copy link
Member

Ma27 commented Feb 20, 2023

Why do we need this? Isn't phpunit pretty dependent on the underlying test framework and thus to be installed with composer?

@drupol
Copy link
Contributor

drupol commented Feb 20, 2023 •
edited
Loading

Why do we need this? Isn't phpunit pretty dependent on the underlying test framework and thus to be installed with composer?

This is a legitimate question, I asked myself the same when I started working with Nix.

IMHO, none of the already packaged PHP Packages (hear: package that are installable with composer) should be in Nix but since there are already some of them (PHPStan, Psalm, GrumPHP, PHPMD, Magerun, PDepend, etc etc), I believe PHPUnit could also be packaged.

I think this could be mitigated since these tools are being provided as a PHAR and therefore considered a arbitrary binary program. This is the reason why I suggested to move them in pkgs/development/tools/misc/ instead.

@onny onny requested a review from drupol February 20, 2023 13:01
@onny onny changed the title phpPackages.phpunit: init at 10.0.9 phpunit: init at 10.0.9 Feb 20, 2023
@onny
Copy link
Contributor Author

onny commented Feb 20, 2023

okay moved it into pkgs/development/tools/misc

drupol
drupol approved these changes Feb 20, 2023
View reviewed changes
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Looks ok for me now

@drupol
Copy link
Contributor

drupol commented Feb 20, 2023

A new phpunit has been released. We need to update this pr

@drupol
Copy link
Contributor

drupol commented Feb 22, 2023

@onny Can you please update the PR so we can move on?

@onny onny changed the title phpunit: init at 10.0.9 phpunit: init at 10.0.11 Feb 22, 2023
@onny onny requested a review from drupol February 22, 2023 11:49
@onny
Copy link
Contributor Author

onny commented Feb 22, 2023

Done :)

drupol
drupol approved these changes Feb 22, 2023
View reviewed changes
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

LGTM

@ofborg ofborg bot requested a review from drupol February 22, 2023 12:12
@onny onny merged commit 0e36aa1 into NixOS:master Feb 22, 2023
@drupol drupol mentioned this pull request Mar 17, 2023
Closed
12 tasks
@dweipert-3138720606 dweipert-3138720606 mentioned this pull request Mar 19, 2023
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drupol drupol drupol approved these changes

@jtojnar jtojnar Awaiting requested review from jtojnar

@aanderse aanderse Awaiting requested review from aanderse

@etu etu Awaiting requested review from etu

@globin globin Awaiting requested review from globin

@Ma27 Ma27 Awaiting requested review from Ma27

@talyz talyz Awaiting requested review from talyz

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@onny @Ma27 @drupol