Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

ci: workaround for the cache bug on Linux #11568

Merged
merged 21 commits into from
Mar 21, 2020

Conversation

ordian
Copy link
Member

@ordian ordian commented Mar 13, 2020

@ordian ordian force-pushed the ao-github-actions-cache-workaround branch from 92edff4 to 769399e Compare March 13, 2020 21:53
@ordian ordian added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Mar 13, 2020
@@ -23,6 +23,10 @@ jobs:
uses: actions/checkout@v2
with:
fetch-depth: 50
# https://github.com/actions/cache/issues/133
- name: Fixup the owner of ~/.cargo/
if: matrix.platform == 'ubuntu-16.04'
Copy link

Choose a reason for hiding this comment

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

You will probably also want to do this on macOS.

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't seem to be a problem for macOS.
Github uses a third-party service for macOS runners: https://help.github.com/en/actions/reference/virtual-environments-for-github-hosted-runners#cloud-hosts-for-github-hosted-runners as opposed to Standard_DS2_v2 VMs on Asure for Linux and Windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bjorn3
Copy link

bjorn3 commented Mar 14, 2020

Cache cargo registry still has the permission problem, while Cache cargo index does work fine. I noticed that I have the same problem with my project. registry and ~/.cargo/bin don't work while index and target dir work fine: https://github.com/bjorn3/rustc_codegen_cranelift/runs/507914716

@ordian
Copy link
Member Author

ordian commented Mar 14, 2020

Cache cargo registry still has the permission problem, while Cache cargo index does work fine. I noticed that I have the same problem with my project. registry and ~/.cargo/bin don't work while index and target dir work fine: https://github.com/bjorn3/rustc_codegen_cranelift/runs/507914716

Wasn't that the case already, it means that this fix doesn't actually work, sadly.

@bjorn3
Copy link

bjorn3 commented Mar 14, 2020

lrwxrwxrwx 1 runner docker 22 Mar 13 15:33 /home/runner/.cargo -> /usr/share/rust/.cargo

~/.cargo is a symlink for /usr/share/rust/.cargo, so the chown only changed the owner of the symlink. Appending a / to the path (sudo chown -R $(whoami):$(id -ng) ~/.cargo/) does fix it without requiring a cache invalidation: https://github.com/bjorn3/rustc_codegen_cranelift/runs/508021777?check_suite_focus=true

@ordian
Copy link
Member Author

ordian commented Mar 14, 2020

@bjorn3 wow thank you for looking into this ❤️

shell: bash
run: git submodule update --init --recursive
run: bash sudo chown -R $(whoami):$(id -ng) ~/.cargo/
Copy link

Choose a reason for hiding this comment

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

This will try to run sudo as shell script I believe. Also windows doesn't have chown, whoami and id. The permission system on Windows works quite different.

Copy link
Member Author

Choose a reason for hiding this comment

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

I expected running bash on Windows would use the Windows Subsystem for Linux 2, but maybe I'm wrong.

Copy link

Choose a reason for hiding this comment

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

bash on windows is just a recompilation of bash for windows I believe. You have to use wsl to run commands on Windows Subsystem for Linux. I assume it is not enabled on gh actions though.

@ordian ordian changed the title ci: workaround for the cache bug ci: workaround for the cache bug on Linux Mar 16, 2020
@ordian ordian added A0-pleasereview 🤓 Pull request needs code review. M1-ci 🙉 Continuous integration. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Mar 16, 2020
Cargo.lock Outdated
@@ -3556,9 +3556,9 @@ dependencies = [

[[package]]
name = "parity-util-mem"
version = "0.5.2"
version = "0.5.1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the downgrade?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not a downgrade compared to master, just reverted unrelated to this PR change (see REVERTME commit).

$acl = Get-Acl "C:\Users\runneradmin\.cargo\"
$accessRule = New-Object System.Security.AccessControl.FileSystemAccessRule("everyone","FullControl","Allow")
$acl.SetAccessRule($accessRule)
$acl | Set-Acl "C:\Users\runneradmin\.cargo\"
Copy link
Member Author

Choose a reason for hiding this comment

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

#11572 was filed for Windows, but it's fine to do here as well

if: matrix.platform == 'windows-latest'
shell: powershell
run: |
$registry-path = "C:\Users\runneradmin\.cargo\registry"
Copy link

Choose a reason for hiding this comment

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

Unexpected token '-path' in expression or statement.
Suggested change
$registry-path = "C:\Users\runneradmin\.cargo\registry"
$registry_path = "C:\Users\runneradmin\.cargo\registry"

I assume _ is allowed.

@bjorn3
Copy link

bjorn3 commented Mar 16, 2020

./index/git.luolix.top-1ecc6299db9ec823/.git/objects/pack/pack-d3f48734b45eac2d457e12f98a6936cf31500e2b.idx: Can't unlink already-existing object
./index/git.luolix.top-1ecc6299db9ec823/.git/objects/pack/pack-d3f48734b45eac2d457e12f98a6936cf31500e2b.pack: Can't unlink already-existing object

if: matrix.platform == 'windows-latest'
shell: powershell
run: |
$registryPath = "C:\Rust\.cargo\registry"
Copy link

Choose a reason for hiding this comment

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

Maybe use

Suggested change
$registryPath = "C:\Rust\.cargo\registry"
$registryPath = "$CARGO_HOME\registry"

to be more resillient against changes to the path? (Not sure if it is correct powershell syntax)

@ordian
Copy link
Member Author

ordian commented Mar 16, 2020

I'm not even sure this is a permission issue on Windows, and the current attempt doesn't seem to fix the problem: https://github.com/openethereum/openethereum/runs/511742121#step:6:7.

So let's try to tackle it in a separate PR?

.github/workflows/build-test.yml Show resolved Hide resolved
.github/workflows/build-test.yml Show resolved Hide resolved
@ordian ordian merged commit 78916be into master Mar 21, 2020
@ordian ordian deleted the ao-github-actions-cache-workaround branch March 21, 2020 14:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. M1-ci 🙉 Continuous integration.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants