Skip to content

The new pip resolver is not backwards compatible#5540

Merged
pavera merged 8 commits intomainfrom
pavera/fix-5405
Aug 16, 2022
Merged

The new pip resolver is not backwards compatible#5540
pavera merged 8 commits intomainfrom
pavera/fix-5405

Conversation

@pavera
Copy link
Copy Markdown
Contributor

@pavera pavera commented Aug 15, 2022

Fixes #5405

Adding error handling and a retry if the pip-compile is not new enough to have the --resolver argument.

@pavera pavera requested a review from a team as a code owner August 15, 2022 17:43
@jurre
Copy link
Copy Markdown
Member

jurre commented Aug 15, 2022

Instead of having to rely on a relatively slow retry, could we somehow detect that it's not available and get it right the first time? 🤔

@pavera
Copy link
Copy Markdown
Contributor Author

pavera commented Aug 15, 2022

Possibly, I took the python approach of "ask forgiveness not permission" initially. I contemplated using the python version which we have at hand though that might not be specific enough for all cases, it would be best if we could use the pip-compile or pip-tools version, but I didn't see a quick/easy way to get that other than shelling out, so went this direction. I'll make another attempt.

Also of note would be we should start seeing the incidence of python 3.6 drop off, and we'll most likely be removing these checks next April (when we hopefully remove 3.6 support), so hopefully the number of times we hit the retry would be low and getting lower.

)
end

def backwards_compat?
Copy link
Copy Markdown
Member

@jurre jurre Aug 16, 2022

Choose a reason for hiding this comment

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

Total nit, would this name make things clearer?

Suggested change
def backwards_compat?
def new_resolver_supported?

(and then flip the logic in the rest of the code)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Probably :)

Comment on lines +112 to +115
def unknown_option_error?(error)
error.message.include?(UNKNOWN_OPTION_ERROR)
end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this code now dead?

Copy link
Copy Markdown
Member

@jurre jurre left a comment

Choose a reason for hiding this comment

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

Overall direction looks great to me, I made a readability suggestion and what I think might be an unused method

end

def new_resolver_supported?
python_version.to_s.start_with?("3.6")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this now be:

Suggested change
python_version.to_s.start_with?("3.6")
!python_version.to_s.start_with?("3.6")

Or what about:

Suggested change
python_version.to_s.start_with?("3.6")
python_version >= Python::Version.new("3.6")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we'd have to do python_version >= Python::Version.new("3.7"). This is the best option I think.

Copy link
Copy Markdown
Member

@jurre jurre left a comment

Choose a reason for hiding this comment

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

@pavera pavera merged commit 1e3ad33 into main Aug 16, 2022
@pavera pavera deleted the pavera/fix-5405 branch August 16, 2022 22:09
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.

Unknown option --resolver in pip-compile for python3.6 + piptools-6.4.0

2 participants