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

Enh/missing constraints #156

Merged
merged 17 commits into from
Mar 15, 2016
Merged

Enh/missing constraints #156

merged 17 commits into from
Mar 15, 2016

Conversation

johntyree
Copy link
Contributor

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:

  • When a user specified job is broken, throw an exception.
  • When a dependency or conflict is broken log a WARNING.

If strict is False:

  • When a dependency or conflict is broken log an INFO.

Closes #149, #144

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.
@johntyree
Copy link
Contributor Author

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a return statement.

Copy link
Contributor Author

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.

@johntyree
Copy link
Contributor Author

Turns out MiniSat was not handling conflict discovery correctly during initialization. It never mattered before because we don't typically have totally broken package metadata. Now that we explicitly ban a package with broken metadata, we can end up with conflicts before we're even done reading all the rules. This fixes that.

@johntyree
Copy link
Contributor Author

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:
Copy link
Contributor

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 ?

@cournape
Copy link
Contributor

The code LGTM, but I have one question regarding the logic

@johntyree
Copy link
Contributor Author

I guess my long comment there didn't work, 😞

The behavior differs depending on whether the broken package is directly
from a job requirement or just from a transitive dependency. Unfortunately,
the only way to know without a large refactor is to count the number of
requirements in the stack.

If there's only one it's a job, Otherwise it isn't.
On Mar 15, 2016 08:11, "David Cournapeau" [email protected] wrote:

The code LGTM, but I have one question regarding the logic


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#156 (comment)

@johntyree
Copy link
Contributor Author

I made the comment a little more explicit. I'll merge after Travis gives the nod.

@cournape
Copy link
Contributor

I understood the comment, but did not get len(requirements) == 1 implied the corresponding requirement was a job requirement.

@cournape
Copy link
Contributor

and I am not sure that I do yet :) I understand that for a job requirement, len(requirements) == 1, but not the opposite relationship

@johntyree
Copy link
Contributor Author

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 egginst on broken packages yourself.

@johntyree
Copy link
Contributor Author

Forcefully installed broken package discussion can move to #158.

Merging this.

johntyree added a commit that referenced this pull request Mar 15, 2016
@johntyree johntyree merged commit 0905488 into master Mar 15, 2016
@johntyree johntyree deleted the enh/missing-constraints branch March 15, 2016 17:16
@cournape cournape modified the milestone: 0.2 Mar 24, 2016
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