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

[FIX] Propagate exception when git url does not match #204

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

StefanRijnhart
Copy link
Contributor

I changed the remote of my git source hoping that mr.developer would reset the code as always-checkout was set to force but it silently continued, leaving the old code in place.

Apparently, the git code raises GitError in most cases but in some cases, such as the case of having to update an unknown branch, it exits with a non zero value. When that happens, queued log messages are discarded, and in non-threading mode, builtout halts without an explanation. What is worse is that in threading mode, buildout continues and eventually reports success.

Replacing the sys.exit by raising a GitError fixes the logging but the exceptions still had to be propagated to the main level in threading mode.

The unknown branch was only encountered by accident. The earlier error of the non-matching remote url was not even considered a fatal error. I'm proposing to do so, at least when always-checkout is set to force.

@StefanRijnhart
Copy link
Contributor Author

sys.exit(1) is found elsewhere in git.py. Should I replace all occurrences?

@fschulze
Copy link
Owner

I only glanced at this and the general idea to replace sys.exit makes sense. I'm unsure about the way the exception is propagated though.

It also seems like this broke Python >= 3.6

@StefanRijnhart StefanRijnhart force-pushed the fix/propagate_exception_in_thread branch from d1ca769 to 3f907e4 Compare March 16, 2021 08:42
@StefanRijnhart
Copy link
Contributor Author

Thank you for your considerations! I will replace the other occurrences of sys.exit(1) then.
As for propagating the exception, I think patching it on the thread is perfectly fine (although I updated the PR this morning to sanitize the threads afterwards). It is a cheapo version of https://stackoverflow.com/a/31614591, but if you like that version better I can refactor accordingly.

Tests are fixed here: #205

@StefanRijnhart StefanRijnhart marked this pull request as draft March 16, 2021 10:18
* Don't drop exception silently (and continue) when it occurs in a forked off thread
* Raise, not exit on unknown git branch
* Raise when git url does not match
@StefanRijnhart StefanRijnhart force-pushed the fix/propagate_exception_in_thread branch from 3f907e4 to 057b7e1 Compare March 16, 2021 12:57
@fschulze
Copy link
Owner

The exception propagation now makes sense to me.

Could you try and add a test for the missing branch case?

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.

2 participants