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 handles broken symlink & unit test added (#2150) #2196

Merged

Conversation

eli-entelis
Copy link
Contributor

pull request related to issue #2150

if there is a match from $PATH of some command, there is a check if the match is a symbolic link, if so we also look at the target. if the target does not exist we ignore it.

a sort of integration test is added which WhichThrowsWhenSymlinkBroken which temporarily creates a symlink that targets a non-existent file "no-such-file-cf7e351f" and also adds to $PATH environment variable the directory of the temporary symlink (which is created at Path.GetTempPath() )

@eli-entelis eli-entelis requested a review from a team as a code owner October 11, 2022 16:44
@eli-entelis eli-entelis force-pushed the whichCommandShouldHandleBrokenSymlinks branch from 5a7ff8a to 385c4f6 Compare October 11, 2022 22:01
@eli-entelis
Copy link
Contributor Author

eli-entelis commented Oct 12, 2022

@nikola-jokic the Runner CI / build (osx-x64) is failing because of some tests with errors : System.IO.FileNotFoundException : tar: command not found
I am not sure if this is because of my changes.
I am running on windows and all the tests are passing. but I see that there is a use of c# preprocessor #if OS_WINDOWS.
Is there a way for me to debug also the not OS_WINDOWS code? because the tests that are failing are throwing exceptions on the not OS_WINDOWS sections of the code.

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

It looks like my suggestion didn't exactly use valid syntax, @Eliminator1999 😅

@eli-entelis
Copy link
Contributor Author

no worries, i will fix it together with the test

@eli-entelis
Copy link
Contributor Author

@fhammerl, @nikola-jokic, I hope I am not bothering you, can you approve the workflow?

@fhammerl
Copy link
Contributor

fhammerl commented Nov 4, 2022

@eli-entelis Glancing at the failing L0 / unit-test, it seems like we fail to get the location of tar on osx. Seems related to this change, tests are still passing on @main.

… test that check symlink that targets a full path; added test that check symlink that targets a relative path;
@fhammerl
Copy link
Contributor

fhammerl commented Nov 8, 2022

@eli-entelis looks like the new tests aren't passing.

Do they have any dependencies on the system you're running?
Have you tried running ./dev.sh test on multiple platforms (osx-x64, linux-x64, win-x64 should be enough)?

@eli-entelis
Copy link
Contributor Author

eli-entelis commented Nov 9, 2022

@fhammerl I was finally able to test another platform and find some issues. hopefully, now it will work.

@eli-entelis
Copy link
Contributor Author

@fhammerl all the tests has passed

Copy link
Contributor

@fhammerl fhammerl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

We'll aim to merge it for the next release.

@eli-entelis
Copy link
Contributor Author

cool! thanks

@TingluoHuang TingluoHuang merged commit 2ecd7d2 into actions:main Apr 7, 2023
nikola-jokic pushed a commit to nikola-jokic/runner that referenced this pull request 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants