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

Register pyenv Python versions #706

Merged

Conversation

danielmitterdorfer
Copy link
Member

With this commit we ensure that make file targets work with all
supported Python versions by registering them in a project-specific file
that pyenv will read during the build.

With this commit we ensure that make file targets work with all
supported Python versions by registering them in a project-specific file
that `pyenv` will read during the build.
@danielmitterdorfer danielmitterdorfer added bug Something's wrong :misc Changes that don't affect users directly: linter fixes, test improvements, etc. labels Jun 7, 2019
@danielmitterdorfer danielmitterdorfer added this to the 1.2.0 milestone Jun 7, 2019
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

Left a suggestion for a different strategy here.

Makefile Outdated
@@ -36,6 +36,8 @@ prereq: make-requirements.txt
pyenv install --skip-existing $(PY37)
# pyenv global system $(PY34) $(PY35) $(PY36) $(PY37)
pyenv global system $(PY35) $(PY36) $(PY37)
@# Ensure all Python versions are registered for this project
@eval "$$(pyenv init -)" && pyenv versions --bare > .python-version;
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed makes sense to just use the already present make-requirements.txt with something like awk -F'=' '{print $2}' make-requirements.txt >.python-version

Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

LGTM, I left a comment for an alternative approach.

Makefile Outdated
@@ -37,7 +37,7 @@ prereq: make-requirements.txt
# pyenv global system $(PY34) $(PY35) $(PY36) $(PY37)
pyenv global system $(PY35) $(PY36) $(PY37)
@# Ensure all Python versions are registered for this project
@eval "$$(pyenv init -)" && pyenv versions --bare > .python-version;
@awk -F'=' '{print $$2 > ".python-version"}' make-requirements.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

This works, but IMHO it is a bit strange to do a file redirection inside awk for this use case;
would it be simpler to
@awk -F'=' '{print $$2}' make-requirements.txt >.python-version

i.e. redirect the final result of awk to .python-version?

Copy link
Member Author

@danielmitterdorfer danielmitterdorfer Jun 7, 2019

Choose a reason for hiding this comment

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

I've changed it now.

Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

:shipit: !

@danielmitterdorfer
Copy link
Member Author

Thanks for the review! :)

@danielmitterdorfer danielmitterdorfer merged commit 6958fe7 into elastic:master Jun 7, 2019
@danielmitterdorfer danielmitterdorfer deleted the register-python-versions branch June 7, 2019 10:16
@dliappis dliappis modified the milestones: 1.2.0, 1.2.1 Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something's wrong :misc Changes that don't affect users directly: linter fixes, test improvements, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants