-
Notifications
You must be signed in to change notification settings - Fork 1
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
Enh/missing constraints #156
Conversation
When strict is True: When a user specified job is broken, throw an exception. When a dependency or conflict is broken log a WARNING. When strict is False: When a dependency or conflict is broken log an INFO.
Still need to check this on windows. I don't know of any broken packages of ours on Linux. |
else: | ||
logger.warning(msg) | ||
else: | ||
logger.info(msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a return
statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, it's down below. We have to make our rule preventing installation first.
Turns out |
This should fix #144 entirely. I've confirmed that on master, it crashes or in some unfortunate cases, doesn't crash or solve the problem, while on this branch it does. There are also tests that replicate the problem and verify that it is caught correctly now. |
# explicitly because we push all of the work through a | ||
# queue. As a proxy, we can examine the associated | ||
# requirements directly. | ||
if len(requirements) == 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the logic behind testing for len(requirements) == 1
?
The code LGTM, but I have one question regarding the logic |
I guess my long comment there didn't work, 😞 The behavior differs depending on whether the broken package is directly If there's only one it's a job, Otherwise it isn't.
|
I made the comment a little more explicit. I'll merge after Travis gives the nod. |
I understood the comment, but did not get |
and I am not sure that I do yet :) I understand that for a job requirement, |
All requirements should either come from a job requirement or because the package is currently installed. If the package is currently installed then it shouldn't be broken unless you're actively trying to destroy the universe (I'll open a new issue for this) by running |
Forcefully installed broken package discussion can move to #158. Merging this. |
This allows us to optionally ignore packages with missing dependencies or conflicts if they aren't tied directly to a job from the user.
If strict is
True
:If strict is
False
:Closes #149, #144