Allow passing configure options without being a real file #235

ruudk · 2021-07-29T11:03:03Z

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 🎉

By removing the `is_file` check we allow installing extensions like this: ```bash 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.

src/Base/Abstracts/Console/Command/BuildCommand.php

pierrejoye · 2021-07-29T11:32:56Z

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 <#235 (comment)>: > @@ -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 <#235 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACE6KGGXHHJ57DS72XCTU3T2EYSVANCNFSM5BGHHWYA> .

ruudk · 2021-07-29T11:35:23Z

@pierrejoye ?

pierrejoye · 2021-07-29T13:59:59Z

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 <#235 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACE6KD4LUCY5MPCXMYXJILT2E4ILANCNFSM5BGHHWYA> .

ruudk · 2021-07-29T14:10:15Z

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 · 2021-08-16T17:11:45Z

Can this be merged?

mlocati · 2021-08-17T09:27:40Z

What about replacing

|| !is_file($force_opts)

with

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

instead of removing that check?

ruudk · 2021-08-17T09:29:09Z

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 · 2021-08-17T09:34:16Z

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 · 2021-08-17T09:37:10Z

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 · 2021-08-17T10:01:55Z

@mlocati Applied your feedback :) Thanks!

ruudk · 2021-08-17T12:10:47Z

Thanks @mlocati 💙

ruudk · 2021-08-17T12:46:24Z

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 · 2021-08-17T13:28:28Z

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 #237

mlocati · 2021-08-17T14:13:59Z

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

ruudk · 2021-08-17T14:14:36Z

@mlocati I'll test it now

ruudk · 2021-08-17T14:16:40Z

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 · 2021-08-17T14:25:50Z

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 · 2021-08-17T14:51:28Z

macOS Big Sur PHP 8.0

mlocati · 2021-08-17T15:42:07Z

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

php -r 'var_dump($argv);' <(echo ciao)

ruudk · 2021-08-18T12:51:16Z

$ 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 · 2021-08-18T13:00:26Z

$ 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 · 2021-08-18T13:02:09Z

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

ruudk · 2021-08-18T13:02:53Z

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 · 2021-08-18T13:04:13Z

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 · 2021-08-18T15:11:58Z

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 · 2021-08-18T15:14:33Z

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 · 2021-08-18T15:20:47Z

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

ruudk · 2021-08-18T15:23:56Z

Thanks for being so persistent in solving this 👏

mlocati · 2021-08-18T15:31:19Z

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

ruudk commented Jul 29, 2021

View reviewed changes

src/Base/Abstracts/Console/Command/BuildCommand.php Outdated Show resolved Hide resolved

Apply feedback

2b86c75

mlocati merged commit ea757d2 into FriendsOfPHP:master Aug 17, 2021

ruudk deleted the allow-fd branch August 17, 2021 12:10

mlocati mentioned this pull request Aug 17, 2021

Fix reading configure options from file description streams #237

Merged

ruudk mentioned this pull request Aug 19, 2021

Fix passing configure options without being a real file #238

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow passing configure options without being a real file #235

Allow passing configure options without being a real file #235

ruudk commented Jul 29, 2021 •

edited

Loading

pierrejoye commented Jul 29, 2021 via email

ruudk commented Jul 29, 2021

pierrejoye commented Jul 29, 2021 via email

ruudk commented Jul 29, 2021

ruudk commented Aug 16, 2021

mlocati commented Aug 17, 2021

ruudk commented Aug 17, 2021

mlocati commented Aug 17, 2021

mlocati commented Aug 17, 2021

ruudk commented Aug 17, 2021

ruudk commented Aug 17, 2021

ruudk commented Aug 17, 2021

mlocati commented Aug 17, 2021

mlocati commented Aug 17, 2021

ruudk commented Aug 17, 2021

ruudk commented Aug 17, 2021

mlocati commented Aug 17, 2021 •

edited

Loading

ruudk commented Aug 17, 2021

mlocati commented Aug 17, 2021

ruudk commented Aug 18, 2021

mlocati commented Aug 18, 2021

ruudk commented Aug 18, 2021

ruudk commented Aug 18, 2021

ruudk commented Aug 18, 2021

mlocati commented Aug 18, 2021

ruudk commented Aug 18, 2021

mlocati commented Aug 18, 2021

ruudk commented Aug 18, 2021

mlocati commented Aug 18, 2021

Allow passing configure options without being a real file #235

Allow passing configure options without being a real file #235

Conversation

ruudk commented Jul 29, 2021 • edited Loading

pierrejoye commented Jul 29, 2021 via email

ruudk commented Jul 29, 2021

pierrejoye commented Jul 29, 2021 via email

ruudk commented Jul 29, 2021

ruudk commented Aug 16, 2021

mlocati commented Aug 17, 2021

ruudk commented Aug 17, 2021

mlocati commented Aug 17, 2021

mlocati commented Aug 17, 2021

ruudk commented Aug 17, 2021

ruudk commented Aug 17, 2021

ruudk commented Aug 17, 2021

mlocati commented Aug 17, 2021

mlocati commented Aug 17, 2021

ruudk commented Aug 17, 2021

ruudk commented Aug 17, 2021

mlocati commented Aug 17, 2021 • edited Loading

ruudk commented Aug 17, 2021

mlocati commented Aug 17, 2021

ruudk commented Aug 18, 2021

mlocati commented Aug 18, 2021

ruudk commented Aug 18, 2021

ruudk commented Aug 18, 2021

ruudk commented Aug 18, 2021

mlocati commented Aug 18, 2021

ruudk commented Aug 18, 2021

mlocati commented Aug 18, 2021

ruudk commented Aug 18, 2021

mlocati commented Aug 18, 2021

ruudk commented Jul 29, 2021 •

edited

Loading

mlocati commented Aug 17, 2021 •

edited

Loading