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

Fix path handling inconsistency in installer caching #230

Conversation

AndreasAlbertQC
Copy link
Contributor

@AndreasAlbertQC AndreasAlbertQC commented May 20, 2022

Problem: The current implementation of installer executable caching is located here. The code aims to determine executablePath, which is supposed to be the full local path (e.g. /path/to/script.extension) to the installer script. However, in the case where the installer script is found in the cache, executablePath is set to the return value of tc.find. This is inconsistent with the goal since tc.find returns a directory name (e.g. /path/to/) without the basename of the actual script. This inconsistency subsequently leads to failures because the executablePath does not end in a known executable extension, which is explicitly checked.

Proposed fix: Manually append the basename of the installer to the executable path in the case where tc.find is used.

Testing / Demonstration: The broken behavior as well as the fixed one can be seen in this test repo, where example 5 from the setup-miniconda readme is run three times with identical job definitions. The only difference between the jobs is the version of setup-miniconda that is used: the tagged v2 version, the HEAD of the develop branch, or my proposed fixed version from this PR. The first two fail with the error described above, the PR version runs without error.

Note: It seems to me that one only encounters this problem with self-hosted github actions runners. When I tried to reproduce the problem with GitHub-hosted runners, I find that the problematic piece of code is never executed because the installer script is never found in the cache, even if a previous job in the same workflow logs a message saying it was cached. It therefore appears that the cache directory is not persistent between jobs in the case of GitHub-hosted runners, and no caching is ever done successfully. While this does not lead to errors, it still seems to be an independent issue of its own.

src/installer/base.ts Outdated Show resolved Hide resolved
@AndreasAlbertQC
Copy link
Contributor Author

Thanks for the suggestion @jonashaag, I changed that line to use path.join

@goanpeca goanpeca added this to the v2.2.0 milestone May 26, 2022
@goanpeca
Copy link
Member

Could you use tha latest develop to fix the lint test @AndreasAlbertQC ?

Thanks!

@goanpeca
Copy link
Member

@AndreasAlbertQC is there an specific issue you are fixing, or you just opened the PR straight away :) ?

@AndreasAlbertQC AndreasAlbertQC force-pushed the 2022-05-19_fix_installer_cache_develop branch from 3b05471 to b81279e Compare May 31, 2022 08:16
@AndreasAlbertQC
Copy link
Contributor Author

@goanpeca I rebased on the current develop now (no other changes).

I am not aware if there is an open issue regarding this problem. I had just encountered it myself and figured it's more efficient to send a PR with a proposed solution rather than open an issue :)

@goanpeca
Copy link
Member

Thanks!

Could you open an issue for this PR? it makes it easier to track things :)

Note: It seems to me that one only encounters this problem with self-hosted github actions runners. When I tried to reproduce the problem with GitHub-hosted runners, I find that the problematic piece of code is never executed because the installer script is never found in the cache, even if a previous job in the same workflow logs a message saying it was cached. It therefore appears that the cache directory is not persistent between jobs in the case of GitHub-hosted runners, and no caching is ever done successfully. While this does not lead to errors, it still seems to be an independent issue of its own.

Also could you open a separate issue for this problem?

Finally thanks a lot for submitting a PR with a fix. We have had several issues with self hosted runners :)

@goanpeca
Copy link
Member

goanpeca commented May 31, 2022

On another note @AndreasAlbertQC and @jonashaag would you be interested in joining the team and help us maintain the project.

We appreciate the contributions and having a users/contributors using self hosted runners would really improve the project and overall experience for users.

Thanks again :)

@jonashaag
Copy link
Contributor

@goanpeca will follow-up using email.

@AndreasAlbertQC
Copy link
Contributor Author

Closes #237

@goanpeca
Copy link
Member

Thanks for the fix @AndreasAlbertQC !

@goanpeca goanpeca merged commit f933706 into conda-incubator:develop Nov 11, 2022
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.

3 participants