Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Process]ExecutableFinder::addSuffix() has no effect#52679
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Process]ExecutableFinder::addSuffix() has no effect#52679
Uh oh!
There was an error while loading.Please reload this page.
Conversation
59697bd to1187d16CompareExecutableFinder::addSuffix() has no effectCan you please check if this bugfix shouldn't be applied to either 5.4 or 6.3 and rebase/retarget if yes? |
1187d16 to912f20dCompare@nicolas-grekas Do you meant you want me to see if the bug already exists in 5.4 or 6.3 and rebase if so? Itdoes already exist in both. I've rebased onto 6.3. Is that what you want? Or do you want me to goall the way back to 5.4? (I thought I understood the bugfix policy on this, but maybe I don't.) |
This option also has no effect on Windows when the As this option has been a no-op for other OS since the creation of the process component, I don't think we should change that as a bugfix. |
Btw, what is your use case for using this on non-Windows systems ? AFAICT, only Windows automatically adds an implicit extension to the executable name when running it. |
I need to look for executables that may end in
That is, in fact, the current behavior--and that's the problem. The in-code documentation gives no indication that the behavior is conditioned on OS, leaving no reason for a user to suspect otherwise: symfony/src/Symfony/Component/Process/ExecutableFinder.php Lines 34 to 39 ind625fa0
In other words, unless users are expected to search the code before usingany and every function in the codebase to see whether it works to their OS, this doesn't behave as advertised. That seems like a bug to me. |
@TravisCarden if you need to find an executable named |
Given that suffixes are totally ignored on non-Windows systems and are ignored on Windows systems that have the |
I can't, because I'm writing a Packagist package, so I don't know that detail about my end users. Isn't that the point of suffixes?
Actually, it will, if I understand you. After applying this PR, and adding $finder =newExecutableFinder();$finder->addSuffix('.phar');$path =$finder->find('composer2');var_dump($path);// string(28) "/usr/local/bin/composer2.phar"$process =newProcess([$path,'--version']);$process->run();var_dump($process->getOutput());// string(43) "Composer version 2.6.5 2023-10-06 10:11:52"
Obviously, that's up to you. As I see it, that would make sense if you were talking about adding new behavior that was never claimed or advertised in the first place. But as I argued above, this is how it claimed to work in the first place, so this would just be making good on a promise it has always made but never kept. |
Rebased,@nicolas-grekas. I have my opinion as to whether this is a bug or not, but I don't really care about the semantics if this could be committed tosome version. If we can't commit it as-is, can someone tell me what it take? |
Uh oh!
There was an error while loading.Please reload this page.
4027764 to1651d3fCompareIt doesn't look like the CI failures are from my changes. Requesting re-review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looking at the CI failure, it does seem that tests are broken by this change:
1) Symfony\Component\Process\Tests\ExecutableFinderTest::testFindWithDefaultSuffixFailed asserting that null matches expected '/home/runner/work/symfony/symfony/src/Symfony/Component/Process/Tests/Fixtures/executable_with_a_default_suffix.bat'.TravisCarden commentedJun 3, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Oh, of course. Because that test asserts on behavior that is now Windows-only. I've just removed it. I'll go ahead and rebase, while I'm at it. |
18e4064 toa2ddf63Comparea2ddf63 tod854b76CompareThank you@TravisCarden. |
Uh oh!
There was an error while loading.Please reload this page.
...except (probably) on Windows.
ExecutableFinder::addSuffix()currently has no effect on non-Windows systems becauseExecutableFinder::find()altogether ignores added suffixes on them. This PR adds tests that prove that it's broken and then fixes it. It clarifies a related docblock along the way.Note: I tried to follow the pattern in similar tests of creating fixture files in the temp directory, but
ExecutableFinder::find()returned a different temp path thansys_get_temp_dir(), rendering path comparison impossible. So I added a few fixture files directly to the repo.