php / pie

The PHP Installer for Extensions
120 stars 2 forks source link

Initial implementation of bin/pie download #11

Closed asgrim closed 2 weeks ago

asgrim commented 4 months ago

Fixes #2

I will merge this on or shortly after 1st July if there is no further show-stopping feedback

Are you an end user who would like to try this out?

Please note that this is just an initial implementation, and will only download an extension to a temporary path on your machine; it will not build or install anything just yet. That is coming soon!

If you understand, and you'd still like to help test this...

Primarily the best way to provide feedback here as an end user of PIE is to check the download command works on your machine! You can do that by following these steps (please adjust for your platform differences of course!)

You should see something like:

$ bin/pie download asgrim/example-pie-extension
You are running PHP 8.3.7
Target PHP installation: 8.3.7 (from /usr/bin/php8.3)
Platform: NonWindows, x86_64, NonThreadSafe
Found package: asgrim/example-pie-extension:1.0.1 which provides ext-example_pie_extension
Extracted asgrim/example-pie-extension:1.0.1 source to: /tmp/pie_downloader_6645f07a28bec9.66045489/asgrim-example-pie-extension-769f906

Are you an extension maintainer who would like to try this out?

It's not strictly necessary to support this just yet, but if you're keen, and please note, things may change before we finally release PIE... so you do this at your own risk. Note: at the moment, you will need to make a new release - see ThePHPF/pie-design#17

More details, for example for help on composer.json, can be read in https://github.com/ThePHPF/pie-design?tab=readme-ov-file#extension-maintainer-register-a-pie-package

Tomyh99095 commented 3 months ago

Fixes #2

asgrim commented 1 month ago

(moved this comment content to the PR description)

derickr commented 1 month ago

I am trying this out. I have been deliberately nitpicky here.

User Interface

bin/pie

It seems the UI is not aware of the size of my terminal window, and it does not wrap text appropriately — this is particularly annoying with the help output.

pie

bin/pie help download

Shows <requested-package-and-version> to mean {ext-name}{?:version-constraint}{?@dev-branch-name} but apparently dev branches need to use :dev-branch-name, the @ is for stability, and # for specific commits.

bin/pie download xdebug/xdebug

Outputs:

Target PHP installation: 8.2.20 (from /usr/local/php/8.2dev/bin/php)
Platform: NonWindows, x86_64, NonThreadSafe

I like how it says what the target installation is, and with which version. If I switch my path around so that php (and friends) point to a different version, that works great too.

The second Platform line sounds all very negative! I understand the reason why it says NotWindows, but it would be better if it said "Linux" — or anything appropriate, especially because that's the main install platform I reckon.

I think the NonThreadSafe word also will confuse people. This is an internal thing really, and most people should not want to have ThreadSafe here.

bin/pie download xdebug/xdebug --with-php-config /usr/local/php/8.3dev/bin/php-config --with-php-path=/usr/local/php/8.2dev/bin/php

I deliberately picked two different PHP versions for --with-php-config and --with-php-path — do we really need both? /usr/local/php/8.3dev/bin/php-config --php-binary already shows the PHP binary path. Or is this only a Windows thing? (In which case, the help output needs to reflect that).

bin/pie download xdebug/xdebug:3.4.0alpha1

Outputs:

  Unable to find an installable package xdebug/xdebug for version 3.4.0alpha1  
  .                                                                            

(Yes, with the . on the next line)

It does not tell me why it can't find it, as there is certainly a release on packagist.

bin/pie download xdebug/xdebug:3.4.0alpha1@alpha

Works, yay!

It downloads it to /tmp/pie_downloader_66599d860c5bc9.02217405/xdebug-xdebug-b8a7e3e — it would be nice that instead of the postfix b8a7e3e it used the git tag associated with that commit hash.

bin/pie download xdebug/xdebug:3.4.0alpha1@beta

Also works... which is odd, as it's an alpha release and not a beta? Maybe packagist/composer lib doesn't care?

Compiling

Compiling works too, by doing:

cd /tmp/pie_downloader_66599d860c5bc9.02217405/xdebug-xdebug-b8a7e3e
phpize && ./configure && make && make install
asgrim commented 1 month ago

Thanks @derickr for your comments, very much appreciated!

It seems the UI is not aware of the size of my terminal window

I have created issue #14 to improve this

Shows <requested-package-and-version> to mean {ext-name}{?:version-constraint}{?@dev-branch-name} but apparently dev branches need to use :dev-branch-name, the @ is for stability, and # for specific commits.

Improved the wording of this in 1188579d96834f9ec309a0dffc9925e79b471dcf

I like how it says what the target installation is, and with which version.

I've collapsed this onto one line in 2298ae7f6958a2f2efb5ca469c21c7dfbef2de81, some examples:

Target PHP installation: 8.3.6 ts, vs16, on Windows x86_64 (from C:\php-8.3.6\php.exe)
Target PHP installation: 8.3.7 nts, on Linux/OSX/etc x86_64 (from /usr/bin/php8.3)

Hopefully this is better?

I deliberately picked two different PHP versions for --with-php-config and --with-php-path — do we really need both?

Strictly speaking, we don't need both, no. We need at least --with-php-path, because on Windows, you don't have php-config (afaik?). But technically, --with-php-path will work on any platform, and --with-php-config will only work where php-config exists. And btw, yes, all we do in PIE is run php-config --php-binary to find the PHP executable. I changed the wording in ff55375d3949a73f9b61003e34e14432dabe780d at least, but we could potentially drop --with-php-config completely; what do you think?

(Yes, with the . on the next line)

I think due to your terminal width; will be addressed in #14 hopefully.

Unable to find an installable package xdebug/xdebug for version 3.4.0alpha1

The reason for this is the stability flags, but I agree this is not immediately clear. I've made #15 to improve this.

It downloads it to /tmp/pie_downloader_66599d860c5bc9.02217405/xdebug-xdebug-b8a7e3e — it would be nice that instead of the postfix b8a7e3e it used the git tag associated with that commit hash.

This is actually the path inside the zip that GitHub gives from the release, so we don't control that part. That said, we could add additional step to move it. Also, we may want to consider using a predictable path, e.g. $HOME/.pie/sources/xdebug/xdebug/3.4.0alpha1/ instead of a random path. Created an issue #16 to discuss this.

asgrim commented 1 month ago

I will merge this on or shortly after 1st July if there is no further show-stopping feedback! Thanks :+1: