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

chore: switch to self hosted runner #458

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore: switch to self hosted runner #458

wants to merge 1 commit into from

Conversation

localcc
Copy link
Contributor

@localcc localcc commented Apr 6, 2024

No description provided.

@Buckminsterfullerene02
Copy link
Member

The only upside this has over the standard github runner is that the VM localcc is using is much more powerful, so the build should go a lot faster.

I had a look at the standard github runner limits and even when linux port is done there's no way we'd ever even get close to the limits.

Since the xmake merge, we have to manually rerun the release workflow 3-5 times just for it to get past the early EOF error. Using a self-hosted runner does not solve this issue. If this continues, it's going to be really annoying to constantly have to babysit the make release actions for 10-20 minutes just to push it past this stupid issue.

Possible fixes:

@UE4SS
Copy link
Collaborator

UE4SS commented Apr 6, 2024

The only upside this has over the standard github runner is that the VM localcc is using is much more powerful, so the build should go a lot faster.

I had a look at the standard github runner limits and even when linux port is done there's no way we'd ever even get close to the limits.

Since the xmake merge, we have to manually rerun the release workflow 3-5 times just for it to get past the early EOF error. Using a self-hosted runner does not solve this issue. If this continues, it's going to be really annoying to constantly have to babysit the make release actions for 10-20 minutes just to push it past this stupid issue.

Possible fixes:

* https://stackoverflow.com/questions/21277806/fatal-early-eof-fatal-index-pack-failed - Lyrth said they tried some of these solutions to no avail

* [Checkout fails on early EOF error. [fetch-pack: invalid index-pack output] actions/checkout#1379 (comment)](https://github.com/actions/checkout/issues/1379#issuecomment-1839483302) - fix that seemed to work for someone here is to use a PAT and switch to http for the fetch, but that's pretty dodgy

* Switch to a different CI pipeline such as azure or jenkins

You sure ?
I checked all the failed actions for the windows-runner branch, and none of them failed because of the EOF error.
There are two different errors:
1 (latest):

ambiguous argument 'HEAD': unknown revision or path not in the working tree.
The process 'C:\Windows\system32\icacls.exe' failed with exit code 1332

2 (previous runs):

Input 'submodules' not supported when falling back to download using the GitHub REST API. To create a local Git repository instead, add Git 2.18 or higher to the PATH.

@localcc
Copy link
Contributor Author

localcc commented Apr 6, 2024

The only upside this has over the standard github runner is that the VM localcc is using is much more powerful, so the build should go a lot faster.

I had a look at the standard github runner limits and even when linux port is done there's no way we'd ever even get close to the limits.

Since the xmake merge, we have to manually rerun the release workflow 3-5 times just for it to get past the early EOF error. Using a self-hosted runner does not solve this issue. If this continues, it's going to be really annoying to constantly have to babysit the make release actions for 10-20 minutes just to push it past this stupid issue.

Possible fixes:

* https://stackoverflow.com/questions/21277806/fatal-early-eof-fatal-index-pack-failed - Lyrth said they tried some of these solutions to no avail
* [Checkout fails on early EOF error. [fetch-pack: invalid index-pack output] actions/checkout#1379 (comment)](https://github.com/actions/checkout/issues/1379#issuecomment-1839483302) - fix that seemed to work for someone here is to use a PAT and switch to http for the fetch, but that's pretty dodgy
* Switch to a different CI pipeline such as azure or jenkins

You sure ?

I checked all the failed actions for the windows-runner branch, and none of them failed because of the EOF error.

There are two different errors:

1 (latest):


ambiguous argument 'HEAD': unknown revision or path not in the working tree.

The process 'C:\Windows\system32\icacls.exe' failed with exit code 1332

2 (previous runs):


Input 'submodules' not supported when falling back to download using the GitHub REST API. To create a local Git repository instead, add Git 2.18 or higher to the PATH.

That was an issue in my runner setup, not the problem we're having with GitHub hosted.

@UE4SS
Copy link
Collaborator

UE4SS commented Apr 6, 2024

That was an issue in my runner setup, not the problem we're having with GitHub hosted.

Okay but I'm not seeing any EOF errors here on github from this branch which I assume is self hosted.
Have you been seeing this error locally ?
I have never seen this error locally myself, which makes me think it's a problem with githubs set up which if true means that we should be able to avoid it if we're self-hosting.

@localcc
Copy link
Contributor Author

localcc commented Apr 6, 2024

EOF errors occur regardless if it's a self hosted or a GitHub hosted runner

@UE4SS
Copy link
Collaborator

UE4SS commented Apr 6, 2024

EOF errors occur regardless if it's a self hosted or a GitHub hosted runner

But how do you know that this is the case if there's been no such error from your runner ?
Is this some sort of known global github error and the runner is locked down in some way even if it's self hosted so you can't fix it ?
Otherwise I don't understand why I literally never have this error locally.
If this error doesn't happen locally then you'd think if we set the self-hosted runner up the same way we'd also not have the error there.

@Buckminsterfullerene02
Copy link
Member

Buckminsterfullerene02 commented Apr 7, 2024

EOF errors occur regardless if it's a self hosted or a GitHub hosted runner

But how do you know that this is the case if there's been no such error from your runner ?
Is this some sort of known global github error and the runner is locked down in some way even if it's self hosted so you can't fix it ?
Otherwise I don't understand why I literally never have this error locally.
If this error doesn't happen locally then you'd think if we set the self-hosted runner up the same way we'd also not have the error there.

This bug has been reported a few times in the GitHub actions repo
actions/checkout#1379

Anyway, can we please just try the org approved PAT and switch to https solution?

@UE4SS
Copy link
Collaborator

UE4SS commented Apr 7, 2024

EOF errors occur regardless if it's a self hosted or a GitHub hosted runner

But how do you know that this is the case if there's been no such error from your runner ?
Is this some sort of known global github error and the runner is locked down in some way even if it's self hosted so you can't fix it ?
Otherwise I don't understand why I literally never have this error locally.
If this error doesn't happen locally then you'd think if we set the self-hosted runner up the same way we'd also not have the error there.

This bug has been reported a few times in the GitHub actions repo actions/checkout#1379

I guess I just don't know how the self-hosted runner works.
I assumed if you host it yourself, you have full control, like it would just ssh in and execute whatever git commands but evidently it's not the case.
My bad.

@UE4SS
Copy link
Collaborator

UE4SS commented Apr 7, 2024

Anyway, can we please just try the org approved PAT and switch to https solution?

I think this is a good idea, but if it doesn't work then I think we should definitely move away from github actions because CI is too important to be failing like this.

@Buckminsterfullerene02
Copy link
Member

Lyrth tested the PAT idea on their fork and there are zero errors across 5 runs, so it seems to be the fix of the problem. The only thing to note is that the owner of the UEPseudo repo (https://github.com/Re-UE4SS) needs to be the one to make the PAT as needs access to the repo in the absence of the SSH key. So Narknon needs to do that.

I think this PR should be closed as we don't have any need for a self hosted runner anymore, as it provides no extra benefit.

@UE4SS
Copy link
Collaborator

UE4SS commented Apr 10, 2024

Lyrth tested the PAT idea on their fork and there are zero errors across 5 runs, so it seems to be the fix of the problem. The only thing to note is that the owner of the UEPseudo repo (https://github.com/Re-UE4SS) needs to be the one to make the PAT as needs access to the repo in the absence of the SSH key. So Narknon needs to do that.

I think this PR should be closed as we don't have any need for a self hosted runner anymore, as it provides no extra benefit.

We should maybe not have only one person with that kind of access to such an important repo, just in case, and didn't you say he was somewhat retired ? So he might be drifting in and out of this place kind of like me, not ideal for the only person with that kind access.

@UE4SS UE4SS closed this Apr 10, 2024
@UE4SS
Copy link
Collaborator

UE4SS commented Apr 10, 2024

Accidentally keyboard shortcutted this PR closed.

@UE4SS UE4SS reopened this Apr 10, 2024
@Buckminsterfullerene02
Copy link
Member

Lyrth tested the PAT idea on their fork and there are zero errors across 5 runs, so it seems to be the fix of the problem. The only thing to note is that the owner of the UEPseudo repo (https://github.com/Re-UE4SS) needs to be the one to make the PAT as needs access to the repo in the absence of the SSH key. So Narknon needs to do that.
I think this PR should be closed as we don't have any need for a self hosted runner anymore, as it provides no extra benefit.

We should maybe not have only one person with that kind of access to such an important repo, just in case, and didn't you say he was somewhat retired ? So he might be drifting in and out of this place kind of like me, not ideal for the only person with that kind access.

We've tried all the other solutions found in various places and this is the only solution that actually works consistantly. So unless you'd prefer to stick with manually coaxing the shit through the pipe for every merge (which typically takes between 10 and 20 minutes) or try to find a better solution, I don't see how we have any choice, despite this choice being pretty shitty (thanks github).

We can use classic PATs which can be set to never expire so theoretically once Narknon should never have to mess with it again. But if he does, it's not like he's so far retired (and I doubt he ever will be) that he won't do it. He's active enough that he's been reading all the PRs and complains about how we're ruining ue4ss, I have no clue what that's on about.

Security wise, the PAT has access to all private repos under the account, so will just have to treat it like an SSH key, i.e. only org admins can view it and have to treat it like a password. Meaning localcc, truman, narknon and you.

image

@UE4SS
Copy link
Collaborator

UE4SS commented Apr 10, 2024

Lyrth tested the PAT idea on their fork and there are zero errors across 5 runs, so it seems to be the fix of the problem. The only thing to note is that the owner of the UEPseudo repo (https://github.com/Re-UE4SS) needs to be the one to make the PAT as needs access to the repo in the absence of the SSH key. So Narknon needs to do that.
I think this PR should be closed as we don't have any need for a self hosted runner anymore, as it provides no extra benefit.

We should maybe not have only one person with that kind of access to such an important repo, just in case, and didn't you say he was somewhat retired ? So he might be drifting in and out of this place kind of like me, not ideal for the only person with that kind access.

We've tried all the other solutions found in various places and this is the only solution that actually works consistantly. So unless you'd prefer to stick with manually coaxing the shit through the pipe for every merge (which typically takes between 10 and 20 minutes) or try to find a better solution, I don't see how we have any choice, despite this choice being pretty shitty (thanks github).

We can use classic PATs which can be set to never expire so theoretically once Narknon should never have to mess with it again. But if he does, it's not like he's so far retired (and I doubt he ever will be) that he won't do it. He's active enough that he's been reading all the PRs and complains about how we're ruining ue4ss, I have no clue what that's on about.

Security wise, the PAT has access to all private repos under the account, so will just have to treat it like an SSH key, i.e. only org admins can view it and have to treat it like a password. Meaning localcc, truman, narknon and you.

image

I don't have a problem with your solution, I just didn't like the idea of only person having admin access to the UEPseudo repo as you implied when you said narknon has to do it.
Maybe I misunderstood something.

@Buckminsterfullerene02
Copy link
Member

Lyrth tested the PAT idea on their fork and there are zero errors across 5 runs, so it seems to be the fix of the problem. The only thing to note is that the owner of the UEPseudo repo (https://github.com/Re-UE4SS) needs to be the one to make the PAT as needs access to the repo in the absence of the SSH key. So Narknon needs to do that.
I think this PR should be closed as we don't have any need for a self hosted runner anymore, as it provides no extra benefit.

We should maybe not have only one person with that kind of access to such an important repo, just in case, and didn't you say he was somewhat retired ? So he might be drifting in and out of this place kind of like me, not ideal for the only person with that kind access.

We've tried all the other solutions found in various places and this is the only solution that actually works consistantly. So unless you'd prefer to stick with manually coaxing the shit through the pipe for every merge (which typically takes between 10 and 20 minutes) or try to find a better solution, I don't see how we have any choice, despite this choice being pretty shitty (thanks github).
We can use classic PATs which can be set to never expire so theoretically once Narknon should never have to mess with it again. But if he does, it's not like he's so far retired (and I doubt he ever will be) that he won't do it. He's active enough that he's been reading all the PRs and complains about how we're ruining ue4ss, I have no clue what that's on about.
Security wise, the PAT has access to all private repos under the account, so will just have to treat it like an SSH key, i.e. only org admins can view it and have to treat it like a password. Meaning localcc, truman, narknon and you.
image

I don't have a problem with your solution, I just didn't like the idea of only person having admin access to the UEPseudo repo as you implied when you said narknon has to do it. Maybe I misunderstood something.

Ah I misunderstood you. Perhaps it would be a good idea for Narknon to give at least one other person the credentials for that account.

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