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

Set .python-version to 3.10 for pyenv users #4456

Merged
merged 4 commits into from
Nov 5, 2024

Conversation

samestep
Copy link
Contributor

@samestep samestep commented Oct 30, 2024

The contribution docs for macOS say to install Python 3.10:

# Install Homebrew tools.
brew install \
bazelisk \
gh \
llvm \
[email protected] \
pre-commit

For people who are using pyenv instead of Homebrew Python, this PR adds a .python-version file to specify Python 3.10 for pyenv to use.

However, I also see that later in the same document, the docs only say that Python >=3.9 is required:

- Carbon requires Python 3.9 or newer.

So in that case, feel free to just close this PR. I also saw that #778 specifically moved Carbon away from pyenv, so if pyenv is discouraged in general, also feel free to just close this.

@zygoloid zygoloid requested review from jonmeow and removed request for zygoloid October 30, 2024 18:26
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

LG -- Sorry, for some reason I'd thought someone else got this PR.

@jonmeow
Copy link
Contributor

jonmeow commented Nov 5, 2024

Sorry, I see the pre-commit issue. In .pre-commit-config.yml, around line 215, I'd suggest inserting an exception, basically:

               .github/pull_request_template.md|
+              .python-version|
               compile_flags.txt|

auto-merge was automatically disabled November 5, 2024 17:04

Head branch was pushed to by a user without write access

@samestep
Copy link
Contributor Author

samestep commented Nov 5, 2024

I just added the copyright text instead, guessing that works too?

@jonmeow
Copy link
Contributor

jonmeow commented Nov 5, 2024

I just added the copyright text instead, guessing that works too?

See the error -- Python's strict about file contents.

auto-merge was automatically disabled November 5, 2024 17:24

Head branch was pushed to by a user without write access

@samestep
Copy link
Contributor Author

samestep commented Nov 5, 2024

Huh, weird... I guess actions/setup-python is buggy, since pyenv says this should be fine:

You can also specify multiple versions in a .python-version file by hand, separated by newlines. Lines starting with a # are ignored.

In any case, I've now implemented your suggestion. Sorry for taking up so much of your time on this!

@jonmeow
Copy link
Contributor

jonmeow commented Nov 5, 2024

In any case, I've now implemented your suggestion. Sorry for taking up so much of your time on this!

No problem, it's really not much time and I should've reviewed last week. FWIW, I wasn't actually looking at what should be supported.

@jonmeow jonmeow added this pull request to the merge queue Nov 5, 2024
Merged via the queue into carbon-language:trunk with commit f03bd6a Nov 5, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants