FriendsOfPHP / pickle

PHP Extension installer
Other
1.65k stars 89 forks source link

Allow passing configure options without being a real file #235

Closed ruudk closed 3 years ago

ruudk commented 3 years ago

By removing the is_file check we allow installing extensions like this:

bin/pickle install --source \
    --with-configure-options <(echo --with-rdkafka=/opt/homebrew/opt/librdkafka) rdkafka

No more need to create a file with the options first 🎉

pierrejoye commented 3 years ago

console option?

On Thu, Jul 29, 2021, 6:04 PM Ruud Kamphuis @.***> wrote:

@.**** commented on this pull request.

In src/Base/Abstracts/Console/Command/BuildCommand.php https://github.com/FriendsOfPHP/pickle/pull/235#discussion_r679050889:

@@ -101,7 +101,7 @@ protected function buildOptions(Package $package, InputInterface $input, OutputI $force_opts = $input->getOption('with-configure-options');

     if ($force_opts) {
  • if (!file_exists($force_opts) || !is_file($force_opts) || !is_readable($force_opts)) {
  • if (!file_exists($force_opts) || !is_readable($force_opts)) {

Does somebody know why this variable is snake_case instead of camelCase?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/FriendsOfPHP/pickle/pull/235#pullrequestreview-717960304, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACE6KGGXHHJ57DS72XCTU3T2EYSVANCNFSM5BGHHWYA .

ruudk commented 3 years ago

@pierrejoye ?

pierrejoye commented 3 years ago

I remember something about console arguments and options using this as a norm. Also in 2021 it can use the defined CS :)

On Thu, Jul 29, 2021, 6:35 PM Ruud Kamphuis @.***> wrote:

@pierrejoye https://github.com/pierrejoye ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/FriendsOfPHP/pickle/pull/235#issuecomment-889043317, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACE6KD4LUCY5MPCXMYXJILT2E4ILANCNFSM5BGHHWYA .

ruudk commented 3 years ago

I remember something about console arguments and options using this as a norm

In Pickle? AFAIK there is no other way than configuration with --with-configure-options <file>.

So I would suggest to merge this, as it allows for a better UX.

ruudk commented 3 years ago

Can this be merged?

mlocati commented 3 years ago

What about replacing

|| !is_file($force_opts)

with

|| !(is_file($force_opts) || is_link($force_opts))

instead of removing that check?

ruudk commented 3 years ago

We can try, but why does this has to be so strict? If it's readable, it's fine, right? Otherwise an error will be thrown for something unexpected.

mlocati commented 3 years ago

PS:

$ php -r 'var_dump(is_file($argv[1]) || is_link($argv[1]));' .
bool(false)

$ php -r 'var_dump(is_file($argv[1]) || is_link($argv[1]));' <(echo 'this is a test')
bool(true)
mlocati commented 3 years ago

If it's readable, it's fine, right?

Because it may be a directory. Another alternative would be to replace || !is_file($force_opts) with || is_dir($force_opts)

ruudk commented 3 years ago

@mlocati Applied your feedback :) Thanks!

ruudk commented 3 years ago

Thanks @mlocati 💙

ruudk commented 3 years ago

I tried it out with 0.7.5 but it doesn't work anymore:

php var/pickle.phar install --dry-run --source --with-configure-options <(echo --with-rdkafka=/opt/homebrew/opt/librdkafka) rdkafka
mlocati commented 3 years ago

I tried it out with 0.7.5 but it doesn't work anymore:

What's the error? Is something like this one?

PHP Warning:  file_get_contents(/dev/fd/<number>):
failed to open stream: No such file or directory
in phar://.../pickle.phar/src/Base/Abstracts/Console/Command/BuildCommand.php on line 112

If so, the error should be fixed by https://github.com/FriendsOfPHP/pickle/pull/237

mlocati commented 3 years ago

@ruudk if you confirm the above error, I'd merge #237 and publish a new pickle version

ruudk commented 3 years ago

@mlocati I'll test it now

ruudk commented 3 years ago

Sorry for not posting the error immediately, that was my intention. This is what I get:

php var/pickle.phar install --dry-run --source --with-configure-options <(echo --with-rdkafka=/opt/homebrew/opt/librdkafka) rdkafka -vvv
warning: An error occurred while redirecting file '--with-rdkafka=/opt/homebrew/opt/librdkafka'
open: No such file or directory
mlocati commented 3 years ago

That's really strange... Here's what I have in an Ubuntu 20.04 with PHP 7.4:

$ curl -sSLf -o /tmp/pickle.phar https://github.com/FriendsOfPHP/pickle/releases/download/v0.7.5/pickle.phar
$ php /tmp/pickle.phar install --dry-run --source --with-configure-options <(echo --with-rdkafka=/opt/homebrew/opt/librdkafka) rdkafka -vvv
+-----------------------------------+---------+
| Package name                      | rdkafka |
| Package version (current release) | 5.0.0   |
| Package status                    | stable  |
+-----------------------------------+---------+
PHP Warning:  file_get_contents(/dev/fd/63): failed to open stream: No such file or directory in phar:///tmp/pickle/src/Base/Abstracts/Console/Command/BuildCommand.php on line 108

Warning: file_get_contents(/dev/fd/63): failed to open stream: No such file or directory in phar:///tmp/pickle/src/Base/Abstracts/Console/Command/BuildCommand.php on line 108

(and that error is fixed by #237)

What's your environment?

ruudk commented 3 years ago

macOS Big Sur PHP 8.0

mlocati commented 3 years ago

@ruudk What do you have if you run this command in a terminal window?

php -r 'var_dump($argv);' <(echo ciao)
ruudk commented 3 years ago
$ php -v
PHP 8.0.9 (cli) (built: Jul 29 2021 17:21:21) ( NTS )
Copyright (c) The PHP Group
Zend Engine v4.0.9, Copyright (c) Zend Technologies
    with Zend OPcache v8.0.9, Copyright (c), by Zend Technologies

$ php -r 'var_dump($argv);' <(echo ciao)
warning: An error occurred while redirecting file 'ciao'
open: No such file or directory
mlocati commented 3 years ago
$ php -r 'var_dump($argv);' <(echo ciao)
warning: An error occurred while redirecting file 'ciao'
open: No such file or directory

So, this is a problem of the mac, not of pickle...

ruudk commented 3 years ago

I don't know how this suddenly happened because when I proposed the PR I obviously tested this and it worked.

ruudk commented 3 years ago

Oke, the problem is with Fish Shell. It works properly in Bash:

bash-3.2$ php -r 'var_dump($argv);' <(echo ciao)
array(2) {
  [0]=>
  string(19) "Standard input code"
  [1]=>
  string(10) "/dev/fd/63"
}
ruudk commented 3 years ago

When I try 0.7.5 in Bash I get:

$ pickle install --dry-run --source --with-configure-options <(echo --with-rdkafka=/opt/homebrew/opt/librdkafka) rdkafka
  - Installing rdkafka (latest-stable): Downloading (100%)
+-----------------------------------+---------+
| Package name                      | rdkafka |
| Package version (current release) | 5.0.0   |
| Package status                    | stable  |
+-----------------------------------+---------+

In BuildCommand.php line 105:

  File '/dev/fd/63' is unusable
mlocati commented 3 years ago
File '/dev/fd/63' is unusable

I have a different error:

file_get_contents(/dev/fd/65): failed to open stream: No such file or directory

Could you check if this works for you?

php -r 'var_dump(file_get_contents(str_replace("/dev/", "php://", $argv[1])));' <(echo ciao)
ruudk commented 3 years ago

Yes!

bash-3.2$ php -r 'var_dump(file_get_contents(str_replace("/dev/", "php://", $argv[1])));' <(echo ciao)
string(5) "ciao
"
bash-3.2$

Also in Fish:

$ php -r 'var_dump(file_get_contents(str_replace("/dev/", "php://", $argv[1])));' (echo ciao | psub)
string(5) "ciao
"
mlocati commented 3 years ago

Great, thanks for confirming! So, #237 should fix the issue

ruudk commented 3 years ago

Thanks for being so persistent in solving this 👏

mlocati commented 3 years ago

I've just published version 0.7.6 with the related patch.