-
Notifications
You must be signed in to change notification settings - Fork 221
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
Mac: Try re-enabling CodeQL on non-legacy build again #2926
Comments
Well, no, it doesn't seem like #2925 is sufficient to avoid this issue, see https://github.com/emlynmac/jamulus/actions/runs/3357222609/jobs/5562884622#step:10:1838 |
Maybe it's still something related. I remember @danryu changing some timeout to 6h. |
I think you're probably referring to the keychain settings timeout in this PR: #2624 Btw I just added signed package validation and upload to the App Store to that PR - the code is working for us. |
I've just commented in #2624: I think that 6h timeout is redundant now that #2927 has been merged (it drops exactly that timeout, AFAIU). That was the initial reason for this PR. :) The best way forward would probably to try to get GUI access. Github shows some hacks to do that, but I haven't got around to try them. |
See #3049 - same thing for Linux now? |
Dropping from 3.10.0. |
Now that the I could raise a PR to make the change if generally agreed. |
What is the current behaviour and why should it be changed?
In #2564 we've moved the CodeQL logic to the legacy build target. The reason was that enabling CodeQL got the build stuck during release signing. The legacy build is not signed, therefore, the problem didn't appear there.
I'm suspecting that it might be the same cause as #2925. In other words: It might just be that CodeQL builds are slower (they are!) and therefore hit the keychain re-lock timeout, which can be worked around.
Describe possible approaches
When #2925 is fixed, we could just re-enable CodeQL for the main build (and disable for legacy).
Has this feature been discussed and generally agreed?
No, but if it works, it should be good to go as it was a workaround only.
The text was updated successfully, but these errors were encountered: