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

Which command should handle broken symlinks #2150

Closed
dhadka opened this issue Sep 22, 2022 · 7 comments
Closed

Which command should handle broken symlinks #2150

dhadka opened this issue Sep 22, 2022 · 7 comments
Assignees
Labels
bug Something isn't working enhancement New feature or request good-first-issue Runner Bug Bug fix scope to the runner

Comments

@dhadka
Copy link
Member

dhadka commented Sep 22, 2022

Describe the bug

https://github.com/actions/runner/blob/main/src/Runner.Sdk/Util/WhichUtil.cs should detect and ignore broken symlinks. This should be considered a bug as calling the native which command will detect and ignore the broken symlink.

To Reproduce

See https://github.com/dhadka/repro-broken-symlink/actions/runs/3106185147/jobs/5032737959 for an example. Here I'm creating a broken symlink for pwsh and then trying to run Powershell using shell: pwsh and through bash with run: pwsh -Command ....

shell: pwsh fails because it's using the custom WhichUtil implementation, matches the broken symlink, and fails with An error occurred trying to start process '/usr/local/bin/pwsh' with working directory '/home/runner/work/repro-broken-symlink/repro-broken-symlink'. No such file or directory

run: pwsh -Command ... works fine as it uses the system's which command that ignores the broken symlink.

Expected behavior

If the target of the symlink does not exist, WhichUtil should skip it and locate any other matches on the PATH.

Runner Version and Platform

Latest Version

OS of the machine running the runner? Ubuntu

What's not working?

See repro above.

Job Log Output

See repro above.

Runner and Worker's Diagnostic Logs

N/A

@dhadka dhadka added the bug Something isn't working label Sep 22, 2022
@nikola-jokic nikola-jokic added the Runner Bug Bug fix scope to the runner label Oct 3, 2022
@nikola-jokic
Copy link
Contributor

Thank you, @dhadka, for reporting this! I applied correct labels and added this issue to the board. ☺️

@fhammerl fhammerl added good-first-issue enhancement New feature or request labels Oct 3, 2022
@fhammerl
Copy link
Contributor

fhammerl commented Oct 3, 2022

We could fail if the path returned by which is a broken symlink.

In other words, after the runner code invokes which to get the path to the executable, we could enforce that it in fact exists.

@eli-entelis
Copy link
Contributor

Hi, I am a first time contributor, I would like to take this issue. Would appreciate any help on where to start

@fhammerl
Copy link
Contributor

fhammerl commented Oct 7, 2022

Hello @Eliminator1999. Let me know if you'd like me to assign you to this issue.

High level goal

Make the runner work the same as which. which ignores broken symlinks, while the runner treats them as if they were executables.

Code location and whatnot

By default, running which -a pwsh would ignore broken symlinks.

  • The runner doesn't run which directly, rather it tries to simulate it.
  • The runner loops through all segments in $PATH and tries to see if the command pwsh is present.
  • Seems like if the result is a symlink, we should try and assert that it points to a real file.
    Consider multi-level symlinks as well.

Pre-requisites

Are you already familiar with how Actions works and how to use self-hosted runners?

Next, I'd recommend checking out our contribution document.
Here's some more specific docs if you're using vscode

@eli-entelis
Copy link
Contributor

thank you @fhammerl, yes please assign this to me, and thanks for all the info!

@eli-entelis
Copy link
Contributor

@fhammerl @nikola-jokic , can someone please review the pr #2196? thank you

TingluoHuang pushed a commit that referenced this issue Apr 7, 2023
* which handles broken symlink & unit test added (#2150)

* Update src/Runner.Sdk/Util/WhichUtil.cs

Co-authored-by: Ferenc Hammerl <31069338+fhammerl@users.noreply.github.com>

* fix pr comments

* trace log added,  in case we find a symlink that is broken

* add check in case the target of the symlink is a relative path; added test that check symlink that targets a full path; added test that check symlink that targets a relative path;

* fix tests

* fix tests for linux

---------

Co-authored-by: Eli Entelis <eli.entelis@hexagon.com>
Co-authored-by: Eli Entelis <42981948+Eliminator1999@users.noreply.github.com>
Co-authored-by: Ferenc Hammerl <31069338+fhammerl@users.noreply.github.com>
@fhammerl
Copy link
Contributor

PR merged

nikola-jokic pushed a commit to nikola-jokic/runner that referenced this issue May 12, 2023
…s#2196)

* which handles broken symlink & unit test added (actions#2150)

* Update src/Runner.Sdk/Util/WhichUtil.cs

Co-authored-by: Ferenc Hammerl <31069338+fhammerl@users.noreply.github.com>

* fix pr comments

* trace log added,  in case we find a symlink that is broken

* add check in case the target of the symlink is a relative path; added test that check symlink that targets a full path; added test that check symlink that targets a relative path;

* fix tests

* fix tests for linux

---------

Co-authored-by: Eli Entelis <eli.entelis@hexagon.com>
Co-authored-by: Eli Entelis <42981948+Eliminator1999@users.noreply.github.com>
Co-authored-by: Ferenc Hammerl <31069338+fhammerl@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request good-first-issue Runner Bug Bug fix scope to the runner
Projects
None yet
Development

No branches or pull requests

4 participants