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

Add composer.lock #5576

Closed
wants to merge 1 commit into from
Closed

Add composer.lock #5576

wants to merge 1 commit into from

Conversation

onny
Copy link

@onny onny commented Nov 25, 2023

On NixOS we introduced a build helper which works great for building PHP applications from source NixOS/nixpkgs#248184
Unfortunately we're requiring the composer.lock file present in the source root directory. For Phpunit we manually added the composer.lock file to our derivation which requires us to update the file with every new version NixOS/nixpkgs@dfa9e40

This PR adds the composer.lock file to the project source.

@drupol

@drupol
Copy link

drupol commented Nov 25, 2023

Thanks @onny for opening the issue, I was about to do it in the near future.

I invite the maintainers of this repo to read the similar issues I opened in some other projects.

@sebastianbergmann
Copy link
Owner

sebastianbergmann commented Nov 27, 2023

Thank you for your contribution. I appreciate the time you invested in preparing this pull request. However, I have decided not to merge it.

A composer.lock file in this repository's root directory is only used when you run composer inside this repository's root directory, in other words: when you work on PHPUnit. How work is done on PHPUnit should be of no concern to users of PHPUnit.

Users of PHPUnit install PHPUnit using one of two mechanisms: they either download a PHP Archive (PHAR) that contains PHPUnit (and its dependencies) from phar.phpunit.de (recommended) or they install PHPUnit using Composer. A composer.lock file in this repository's root directory is not used in either of this cases.

I think that you should consider "installing" / using PHPUnit from a PHAR instead of installing it using Composer.

@drupol
Copy link

drupol commented Nov 27, 2023

Hi,

I understand. However, I believe this request is primarily aimed at enhancing PHPunit's integration and ensuring long-term reliability for package managers like apt, rpm, yum, etc.

Introducing a composer.lock file into the phpunit repository would guarantee that phpunit is reproducible bit-for-bit on any system, regardless of its environment. This file is crucial for ensuring such reproducibility. When you distribute a "binary", confirming that this binary is reproducible is critical for both reliability and, to some extent, security.

I understand your suggestion to use the PHAR file, which represents the "compiled" output. However, in Nix/NixOS (or any other), a source-based distribution, our preference is to compile everything rather than rely on pre-compiled binaries. Without a composer.lock file, we can't assure reproducibility for PHPUnit. This underpins our request.

As a less demanding alternative, could you consider publishing the composer.lock file used to build the PHAR along with the PHAR?

What do you think?

@sebastianbergmann
Copy link
Owner

Let me start by saying that the information is already there:

$ wget https://phar.phpunit.de/phpunit.phar
$ php phpunit.phar --manifest
phpunit/phpunit: 10.4.2
myclabs/deep-copy: 1.11.1
nikic/php-parser: v4.17.1
phar-io/manifest: 2.0.3
phar-io/version: 3.2.1
phpunit/php-code-coverage: 10.1.7
phpunit/php-file-iterator: 4.1.0
phpunit/php-invoker: 4.0.0
phpunit/php-text-template: 3.0.1
phpunit/php-timer: 6.0.0
sebastian/cli-parser: 2.0.0
sebastian/code-unit: 2.0.0
sebastian/code-unit-reverse-lookup: 3.0.0
sebastian/comparator: 5.0.1
sebastian/complexity: 3.1.0
sebastian/diff: 5.0.3
sebastian/environment: 6.0.1
sebastian/exporter: 5.1.1
sebastian/global-state: 6.0.1
sebastian/lines-of-code: 2.0.1
sebastian/object-enumerator: 5.0.0
sebastian/object-reflector: 3.0.0
sebastian/recursion-context: 5.0.0
sebastian/type: 4.0.0
sebastian/version: 4.0.1
theseer/tokenizer: 1.2.1
$ php phpunit.phar --sbom    
<?xml version="1.0"?>
<bom xmlns="http://cyclonedx.org/schema/bom/1.4">
 <components>
  <component type="library">
   <group>phpunit</group>
   <name>phpunit</name>
   <version>10.4.2</version>
   .
   .
   .
 </components>
</bom>

The composer.lock file used to build the PHAR is not (yet) part of the PHAR. This can, and I think should (regardless of your use case) be changed. Progress on this is tracked in #5577.

When you distribute a "binary", confirming that this binary is reproducible is critical for both reliability and, to some extent, security.

I understand that. I have been trying to raise awareness for software supply chain issues through presentations for years.

So far, though, it had not occurred to me that putting composer.lock under version control might be relevant here. And I am still not convinced that it is.

I believe this request is primarily aimed at enhancing PHPunit's integration and ensuring long-term reliability for package managers like apt, rpm, yum, etc.

And this is why: I do not think that PHPUnit should be installed using an operating system package manager. Quoting from PHPUnit's documentation:

Using such a global installation of PHPUnit is almost always a bad idea as the different projects you work on may require different versions of PHPUnit, for instance.

It is therefore best to use a project-local installation of the version of PHPUnit that should be used for the project at hand.

Consequently, the package manager of your operating system should not be used to install PHPUnit as this would result in a global installation of PHPUnit.

@sebastianbergmann
Copy link
Owner

#5577 has been implemented and the next releases of PHPUnit 8.5, PHPUnit 9.6, and PHPUnit 10 (likely PHPUnit 10.5.0) will have a --composer-lock CLI option for the PHAR binary.

I will work on #5578 ASAP and put composer.lock under version control.

@drupol
Copy link

drupol commented Nov 27, 2023

The composer.lock file used to build the PHAR is not (yet) part of the PHAR. This can, and I think should (regardless of your use case) be changed. Progress on this is tracked in #5577.

👍

I understand that. I have been trying to raise awareness for software supply chain issues through presentations for years.

It's unfortunate we didn't meet at IPC Munich; discussing this over a drink would have been enlightening!

So far, though, it had not occurred to me that putting composer.lock under version control might be relevant here. And I am still not convinced that it is.

This is relevant when someone else wants to reproduce locally the PHAR file. Without it, it's not possible to guarantee the reproducibility.

And this is why: I do not think that PHPUnit should be installed using an operating system package manager.

Regarding PHPUnit's installation: The primary goal here is not to encourage widespread installation via OS package managers. Instead, it's about ensuring reproducibility, which is a separate issue.

Linux offers numerous ways to deploy a "binary" (app), and this request isn't about altering that. However, Nix uniquely allows defining "per project" dependencies, independent of the PHP version used in your project. This capability is demonstrated in the screencast at slide 95 of this year's IPC presentation, showcasing a project using PHP 5.6 while running PHPStan on PHP 8.

Anyway, I'm looking forward to seeing how you're going to include the composer.lock file, I'm extremelly happy that this discussion is going in the good direction.

Have a good day.

@sebastianbergmann
Copy link
Owner

I think there is/was a "race condition" and you did not read #5576 (comment) before writing #5576 (comment) (or maybe I was not clear enough).

TL;DR: The composer.lock file will be put under version control.

@drupol
Copy link

drupol commented Nov 27, 2023

Indeed! I started my message in the train and finished it at work, I didn't see the intermediate comment.

Glad to read that you're going to put it under VC ! Cool !

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

Successfully merging this pull request may close these issues.

None yet

3 participants