-
Notifications
You must be signed in to change notification settings - Fork 38
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(cache): wrong check for symbolic link #89
fix(cache): wrong check for symbolic link #89
Conversation
-f FILE FILE exists and is a regular file -L FILE FILE exists and is a symbolic link (same as -h) issue: CircleCI-Public#83 (comment)
Thanks for surfacing this! I do wonder why |
For the bug that I corrected here! it is good on my side, I just tested. ✅ 🎉 👀 I think there is a second problem with the checksum of the file. python-orb/src/commands/install-packages.yml Line 114 in 349f0de
You can do a second PR for this problem 👍 |
BTW, the checksum of a symlink would really change if the content of the linked file changes? Maybe a hard link (if possible, but may not work on all executors) or a copy would be more accurate? |
Currently we are hardlinking, however, I think I would like to change this because of this issue here: CircleCI-Public/node-orb#119 That's a good point though, and would be would be pretty easy to test. I'll get back to you with the results of that. @clementlecorre Can you send me a link to your build please so I can see if there might be anything else interfering with this? It's strange that our integration tests are succeeding so I'm wondering if you may be hitting an edge case or that your environment may be different? |
Here you can see that there are no differences between the sym and hard link as it follows the link to the source file, even if the contents of the file change. |
@Jaryt I just make a repo where I reproduce the problem. I did another test on the checksum and I think it is not a bug of the function |
@clementlecorre I found the source of the issue. This is occurring due to the |
Found another issue that was tangential to this that I initially over-looked. Going to close this out in favor of #90 Thanks for the assistance here! ❤️ |
@Jaryt I just did a new test and I still have the problem with v2.0.1
|
@clementlecorre Thanks for checking. Can you add |
@Jaryt Indeed it works with a caching reset! Thanks for your feedback! |
Problem at "Copy to cache directory" step
issue: #83 (comment)