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

Initial implementation of bin/pie download #11

Merged
merged 43 commits into from
Jul 9, 2024

Conversation

asgrim
Copy link
Collaborator

@asgrim asgrim commented Mar 25, 2024

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!)

  • Ensure you are using PHP 8.3 (this is because my test extension asgrim/example-pie-extension is intentionally only compatible with PHP 8.3)
  • git clone -b download-implementation https://github.com/asgrim/pie.git
  • composer install
  • bin/pie download asgrim/example-pie-extension
  • Let me know if you encounter any errors or something unexpected!

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

@asgrim asgrim added the enhancement New feature or request label Mar 25, 2024
@asgrim asgrim self-assigned this Mar 25, 2024
src/DependencyResolver/ResolveDependencyWithComposer.php Outdated Show resolved Hide resolved
src/Downloading/DownloadZip.php Outdated Show resolved Hide resolved
src/Downloading/ExtractZip.php Show resolved Hide resolved
src/Container.php Outdated Show resolved Hide resolved
src/Container.php Outdated Show resolved Hide resolved
@Tomyh99095

This comment was marked as off-topic.

@asgrim asgrim force-pushed the download-implementation branch 4 times, most recently from b5987c2 to 749eb05 Compare April 24, 2024 10:37
@asgrim asgrim changed the title [WIP] Download implementation Initial implementation of bin/pie download May 16, 2024
src/Command/DownloadCommand.php Outdated Show resolved Hide resolved
test/integration/Command/DownloadCommandTest.php Outdated Show resolved Hide resolved
@asgrim asgrim marked this pull request as ready for review May 27, 2024 12:25
@asgrim
Copy link
Collaborator Author

asgrim commented May 27, 2024

(moved this comment content to the PR description)

@derickr
Copy link
Member

derickr commented May 31, 2024

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
Copy link
Collaborator Author

asgrim commented Jun 3, 2024

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 1188579

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

I've collapsed this onto one line in 2298ae7, 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 ff55375 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
Copy link
Collaborator Author

asgrim commented Jun 24, 2024

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

@asgrim asgrim merged commit c3e710e into php:main Jul 9, 2024
14 checks passed
@asgrim asgrim deleted the download-implementation branch July 9, 2024 19:37
@asgrim asgrim added this to the 0.1.0 - initial release milestone Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Download command & related components
5 participants