-
Notifications
You must be signed in to change notification settings - Fork 49
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
Checking for gpg
availability sometimes times out
#428
Comments
Sometimes, in some systems, not pointing any fingers but Windows, just finding out that there's no `gpg` available takes >3 seconds. In that case, we now just assume that there's no GPG because a binary that takes 3 seconds even to start isn't any better than no binary at all. Fixes secure-systems-lab#428.
Sometimes, in some systems, not pointing any fingers but Windows, just finding out that there's no `gpg` available takes >3 seconds. In that case, we now just assume that there's no GPG because a binary that takes 3 seconds even to start isn't any better than no binary at all. Fixes secure-systems-lab#428. Signed-off-by: Zachary Newman <[email protected]>
Hm, I'm not sure if this is the best way to handle the issue because unless we're very careful, we'd end up not running GPG tests on a Windows box but the CI still goes green, meaning we merge something we shouldn't. There are a number of tests gated by the |
Alternatives might be:
|
Ping—do any of the above sound acceptable? |
Agree with @adityasaky. I like the plan to not execute processes on import, what do you think @lukpueh ? |
I think it's a good idea to assert Evaluating HAVE_GPG and GPG_COMMAND lazily when needed seems fine, but we might want to cache the result so that we don't have to run the commands more often than necessary. Background: |
See secure-systems-lab#428. This commit is over-complicated, but (mostly) not breaking (you can continue to use `HAVE_GPG` and `GPG_COMMAND`). In this commit: - Add cacheing (`functools.lru_cache`) to `is_available_gnupg`. - Add `gpg_command()` to replace `GPG_COMMAND`. - Add `have_gpg()` to replace `HAVE_GPG`. - Replace `GPG_COMMAND` and `HAVE_GPG` with a `lazy.wrap_thunk()` wrapper for their corresponding functions. - This wrapper lazily runs the underlying thunk and passes through *everything* to the result of calling it. - This is a terrible hack and I'm sorry. - Some things still break, like `StringIO`. - Add a test that imports `securesystemslib.constants.gpg` with a busted `$GNUPG` to make sure that importing the library doesn't try to shell out. This failed before this change. Signed-off-by: Zachary Newman <[email protected]>
Fixes secure-systems-lab#428. Okay to do this because we will fail tests if GPG is unexpectedly unavailable (secure-systems-lab#434). Signed-off-by: Zachary Newman <[email protected]>
See secure-systems-lab#428. This commit is over-complicated, but (mostly) not breaking (you can continue to use `HAVE_GPG` and `GPG_COMMAND`). In this commit: - Add cacheing (`functools.lru_cache`) to `is_available_gnupg`. - Add `gpg_command()` to replace `GPG_COMMAND`. - Add `have_gpg()` to replace `HAVE_GPG`. - Replace `GPG_COMMAND` and `HAVE_GPG` with a `lazy.wrap_thunk()` wrapper for their corresponding functions. - This wrapper lazily runs the underlying thunk and passes through *everything* to the result of calling it. - This is a terrible hack and I'm sorry. - Some things still break, like `StringIO`. - Add a test that imports `securesystemslib.constants.gpg` with a busted `$GNUPG` to make sure that importing the library doesn't try to shell out. This failed before this change. Signed-off-by: Zachary Newman <[email protected]>
https://github.com/secure-systems-lab/securesystemslib/actions/runs/3362099110/jobs/5573366570
This was on MacOS. The 3.043s looks suspiciously like one default timeout. |
Weird that it's happening on Mac, it was always Windows before. I wonder whether it's just a weird virtualization hiccup where reading the binary from disk takes a really long time. Maybe we should consider increasing the timeout? |
When importing
constants.py
,securesystemslib
checks for GPG by callingis_available_gnupg()
"https://github.com/secure-systems-lab/securesystemslib/blob/v0.24.0/securesystemslib/gpg/constants.py#L26-L32
In go-tuf, we've seen this time out a few times. For instance,
https://github.com/theupdateframework/go-tuf/actions/runs/3100521501/jobs/5020899719
You might think: that's ridiculous! How would a call to GPG time out? And the answer is: GitHub's hosted Windows runners experience intermittent extreme slowness. I don't believe GPG is even present on these machines, so it's really Windows taking >3 seconds to tell you that the binary is not found. Regardless, it happens.
My preference would be to avoid doing this on import at all, but I think just catching
subprocess.TimeoutExpired
would fix it.The text was updated successfully, but these errors were encountered: