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

Related to issue #1236 and RequirementChecker section #1237

Open
llaville opened this issue Dec 1, 2023 · 9 comments
Open

Related to issue #1236 and RequirementChecker section #1237

llaville opened this issue Dec 1, 2023 · 9 comments

Comments

@llaville
Copy link
Contributor

llaville commented Dec 1, 2023

Why don't we see all extensions declared on root composer.json file

See with GitHub repo :

Built online for the first time at https://github.com/overtrue/phplint/actions/runs/7056960071/job/19209802647

But when we check with BOX info command, we got :


API Version: 1.1.0

Archive Compression: None
Files Compression: None

Signature: SHA-1
Signature Hash: C72ECCE5306E94D119732B10D34311C30F993AC4

Metadata: None

Timestamp: 1701414157 (2023-12-01T07:02:37+00:00)

RequirementChecker:
  Required:
  - PHP ^8.0 (root)
  - ext-json (root)
  Conflict:
  - ext-psr (symfony/service-contracts)

Contents: 459 files (2.36MB)

 // Use the --list|-l option to list the content of the PHAR.

Only json extension is displayed, but not mbstring. Is there a reason or is it just a bug ?

@llaville
Copy link
Contributor Author

llaville commented Dec 1, 2023

@theofidry
Copy link
Member

Could there be that a mbstring poly fill is present? Thanks to the composer.lock file we remove all the covered extension requirements

@llaville
Copy link
Contributor Author

llaville commented Dec 2, 2023

Personnaly I'd like the Composer Unused solution that display it even if the symfony/polyfill-mbstring is present

[email protected] in /shared/backups/github/phplint $ /shared/backups/php/composer-unused.phar --version
composer-unused 0.8.11
[email protected] in /shared/backups/github/phplint $ /shared/backups/php/composer-unused.phar


Results
-------

Found 10 used, 0 unused, 0 ignored and 0 zombie packages

 Used packages
 ✓ php
 ✓ ext-json
 ✓ ext-mbstring
 ✓ symfony/cache (https://github.com/symfony/cache)
 ✓ symfony/console (https://github.com/symfony/console)
 ✓ symfony/event-dispatcher (https://github.com/symfony/event-dispatcher)
 ✓ symfony/finder (https://github.com/symfony/finder)
 ✓ symfony/options-resolver (https://github.com/symfony/options-resolver)
 ✓ symfony/process (https://github.com/symfony/process)
 ✓ symfony/yaml (https://github.com/symfony/yaml)

 Unused packages

 Ignored packages

 Zombies exclusions (did not match any package)

@llaville
Copy link
Contributor Author

@theofidry I've another use case that I don't understand origin.

I've recently played with PHP Benchmarking framework v1.12.15

And I've noticed that php extensions declared does not match with the box info:general command

API Version: 1.1.0

Built with Box: 4.3.8@5534406

Archive Compression: None
Files Compression: GZ

Signature: SHA-1
Signature Hash: AC745626F21904EBC3B1FB30A428886EDF13F4B8

Metadata: None

Timestamp: 1701260649 (2023-11-29T12:24:09+00:00)

RequirementChecker:
  Required:
  - PHP ^8.1 (root)
  - ext-zlib (root)
  - ext-dom (root)
  - ext-json (root)
  - ext-pcre (root)
  - ext-reflection (root)
  - ext-spl (root)
  - ext-tokenizer (root)
  Conflict:
  - ext-psr (root)

Contents: 907 files (821.25KB)

 // Use the --list|-l option to list the content of the PHAR.

Extension zlib is shown as a root definition, while it's not the case in PHPBench composer.json

Confirmed by the composer why ext-zlib command that told us that

There is no installed package depending on "ext-zlib"

Any idea of where it come from ?

@theofidry
Copy link
Member

theofidry commented Dec 10, 2023

Extension zlib is shown as a root definition, while it's not the case in PHPBench composer.json

Probably from:

Files Compression: GZ

If it's GZ-compressed it requires ext-zlib.

Currently in .requirements.php the source key is filled by either the package name or null for the root. Maybe this could be changed in favour of 'compression algorithm' for this case for clarity

@theofidry
Copy link
Member

@llaville do you think it would be good enough to show this as part of a command rather than the info? The difference would be that you compute the requirements before creating the PHAR, as opposed to the info which shows the requirements shipped. This would offer two benefits:

  • The output can be changed an more verbose, e.g. to accommodate for your request.
  • Adding more information does not come at the cost of changing the requirement checker shipped.
  • This allows you to inspect the requirements of an app even if you do not ship the requirement checker.

WDYT?

@llaville
Copy link
Contributor Author

@theofidry
On my project https://github.com/llaville/umlwriter/tree/3.4.0

By invoking composer why ext-zlib command, I get only

jawira/plantuml-encoding v1.1.0 requires ext-zlib (*)

But when I compile project with BOX 4.6.0@95b0d8e, and run box info command, I get :

API Version: 1.1.0

Built with Box: 4.6.0@95b0d8e

Archive Compression: None
Files Compression: GZ

Signature: SHA-512
Signature Hash: DAC919F2C1208C8D9797B2AFB978C6BF3E4E843A976D359766AF9BA0C1D555C289A5A59AEFE921A6964ED00571876D112FB3B95BC534E5E0C86577FEB97E6AB2

Metadata: None

Timestamp: 1702541592 (2023-12-14T08:13:12+00:00)

RequirementChecker:
  Required:
  - PHP ^8.0 (root)
  - ext-zlib (root)
  - ext-zlib (jawira/plantuml-encoding)
  - ext-tokenizer (nikic/php-parser)
  - ext-json (roave/better-reflection)
  Conflict:
  - ext-psr (symfony/service-contracts)

Contents: 1548 files (12.27MB)

 // Use the --list|-l option to list the content of the PHAR.

As the zlib extension is not defined into the composer.json file of the project, the mention ext-zlib (root) lead to confusion for me !

I'd like to see rather mention like ext-zlib (suggested by humbug/box because files compression is enabled)

That will give something better understandable (but it's only my opinion)

RequirementChecker:
  Required:
  - PHP ^8.0 (root)
  - ext-zlib (suggested by humbug/box because files compression is enabled)
  - ext-zlib (jawira/plantuml-encoding)
  - ext-tokenizer (nikic/php-parser)
  - ext-json (roave/better-reflection)
  Conflict:
  - ext-psr (symfony/service-contracts)

@llaville
Copy link
Contributor Author

llaville commented Dec 14, 2023

Another use case that lead for me to a confusion, is when an extension is defined as root in the composer.json but did not shown on the RequirementChecker section of the box info, just because there is a polyfill installed.

RequirementChecker:
  Required:
  - PHP ^8.1 (root)
  - ext-dom (root)
  - ext-json (root)
  Conflict:
  - ext-psr (symfony/service-contracts)

On project https://github.com/overtrue/phplint/tree/9.1, the mbstring extension is declared, but does not appear on box info report. And polyfill is only installed by other dependencies, but not declared on root project.

With composer why symfony/polyfill-mbstring, we get on PHP 8.1 platform :

symfony/console v6.4.1 requires symfony/polyfill-mbstring (~1.0)
symfony/string  v6.4.0 requires symfony/polyfill-mbstring (~1.0)

Compare with composer-unused report that did not list the polyfill (installed) but show the mbstring extension

Results
-------

Found 11 used, 0 unused, 0 ignored and 0 zombie packages

 Used packages
 ✓ php
 ✓ ext-dom
 ✓ ext-json
 ✓ ext-mbstring
 ✓ symfony/cache (https://github.com/symfony/cache)
 ✓ symfony/console (https://github.com/symfony/console)
 ✓ symfony/event-dispatcher (https://github.com/symfony/event-dispatcher)
 ✓ symfony/finder (https://github.com/symfony/finder)
 ✓ symfony/options-resolver (https://github.com/symfony/options-resolver)
 ✓ symfony/process (https://github.com/symfony/process)
 ✓ symfony/yaml (https://github.com/symfony/yaml)

 Unused packages

 Ignored packages

 Zombies exclusions (did not match any package)

@theofidry
Copy link
Member

I'd like to see rather mention like ext-zlib (suggested by humbug/box because files compression is enabled)

Yes that can be added, not sure if with the exact workding. Note that this is explicitly said in the build output already.

Another use case that lead for me to a confusion, is when an extension is defined as root in the composer.json but did not shown on the RequirementChecker section of the box info, just because there is a polyfill installed.

I totally understand the point, but in #1237 (comment), my question is if having it as part of another command would be good enough.

The reason why I suggest another command is that it's a lot easier and cheaper to add information as part of a Box command. The produced requirement checker is shipped as part of the final PHAR so I prefer to keep the shipped code as stable and light as possible. Which is why listing all the intermediate steps of finding the requirements makes me a bit uneasy.

theofidry added a commit to theofidry/box that referenced this issue Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants