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

@tknerr metadata deps not honored #717

Merged
merged 5 commits into from Jul 25, 2013

Conversation

sethvargo
Copy link
Contributor

Originally #712, but updated to conform to our test structure. Merge this instead of #712, but let's keep the conversation there. (I think merging this will automatically close #712).

Since this is a branch on RiotGames/berkshelf, anyone on the team can push and make changes.

@@ -436,3 +436,49 @@ Feature: install cookbooks from a Berksfile
"""
Unknown site shortname 'somethingabsurd' - supported shortnames are:
"""

@focus
Copy link
Contributor

Choose a reason for hiding this comment

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

gotta remove the focus bits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left them in until we can figure out how to fix them (i.e. this PR is incomplete)

@sethvargo
Copy link
Contributor Author

@reset is this fixed with #693?

@reset
Copy link
Contributor

reset commented Jul 16, 2013

@sethvargo it should be, you might want to re-write these tests to work with the Berks API server stuff that I introduced and check it out.

Installing bacon (0.2.0)
"""

@focus
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@reset can you take a look at this? It's still failing against master. It looks like we still trust local versions...

Copy link
Contributor

Choose a reason for hiding this comment

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

this one too

@ivey
Copy link
Contributor

ivey commented Jul 18, 2013

per triage, @andrewGarson to take a look

@andrewGarson
Copy link
Contributor

This looks like a different issue than we have seen in the past.

The old resolver would resolve each dependency in order, and if you happened to grab a version of some cookbook that didn't work with a future dependency it didn't care. That's why reordering your dependencies worked.

Reordering these dependencies in the test doesn't 'fix' the problem. Something more complex is happening. It looks like some dependency is not being respected at all or a version compare is going awry.

Still investigating.

based upon the version_constraint even though we have already resolved
the graph and know what the appropriate version is. This was causing it
to grab versions that were too new.

Bump ridley version to match master so that bundler will be happy
@andrewGarson
Copy link
Contributor

@sethvargo @reset @ivey please take a look at this

#cached_cookbook was satisfying based upon the version_constraint instead of the locked_version of a dependency. This was causing berkshelf to resolve the dependencies properly, but then lookup the wrong cached cookbook.

@sethvargo
Copy link
Contributor Author

@andrewGarson holy fuck shit. Great find. I don't know how many hours I spent tracing code. We should consider back-porting this and releasing our last 2.0 release.

It looks like the build is failing on dependency#locked_version for some reason though. Infinite stack trace 🔬

@tknerr
Copy link
Contributor

tknerr commented Jul 23, 2013

We should consider back-porting this and releasing our last 2.0 release.

Yeah! 👍 :-)

@andrewGarson
Copy link
Contributor

I can't recreate that failing test locally for some reason and there is no obvious reason for the stack to be blown that way

@andrewGarson
Copy link
Contributor

Just found the problem causing the failure, will work on a fix.

@reset
Copy link
Contributor

reset commented Jul 24, 2013

@andrewGarson this looks good, 👍 soon as you get that fix in for the failing tests.

in #locked_version and #cached_cookbook. This is removed.

Instead of getting the cached_cookbook to find the version, just read
locked_version
@andrewGarson
Copy link
Contributor

@reset @sethvargo you should take a look at this again before merging.

@@ -407,3 +407,56 @@ Feature: install cookbooks from a Berksfile
An error occurred during Git execution:
"""
And the exit status should be "GitError"

@focus
Copy link
Contributor

Choose a reason for hiding this comment

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

gotta remove this focus tag

@tknerr
Copy link
Contributor

tknerr commented Jul 25, 2013

@andrewGarson cool stuff, thanks! 👍

Btw: will this be backported to 2-0-stable or only fixed in master / 3.0?

@sethvargo
Copy link
Contributor Author

👍 after removing the @Focus tags. Good catch on the infinite recursion 😄

andrewGarson added a commit that referenced this pull request Jul 25, 2013
@andrewGarson andrewGarson merged commit 7ba65aa into master Jul 25, 2013
@sethvargo sethvargo deleted the tknerr-metadata-deps-not-honored branch July 25, 2013 20:13
@sethvargo
Copy link
Contributor Author

@tknerr this cannot be easily backported, as the fix is not the same as what @andrewGarson came up with (since the resolver has changed so much)

@tknerr
Copy link
Contributor

tknerr commented Jul 25, 2013

@sethvargo ok, no problem, just let me know when the next 3.0 pre version is out

@berkshelf berkshelf locked and limited conversation to collaborators Jun 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants