-
Notifications
You must be signed in to change notification settings - Fork 522
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
add support for python-version-file #336
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect different approach - specify python-version: 'file'
to use file as version source.
I think the presence of the |
Ok, I see what you are trying to achieve.
Ruby has a nice 'default to file' if version is unspecified which could be handy, but should be a separate PR https://github.com/ruby/setup-ruby/blob/master/action.yml#L9 |
Right - I tried that at first, and would have kept it since I was mostly copying that code anyway. But we can't do that here because
Yep, agreed. I'll update that.
Also agreed. If it weren't for |
@adilosa Hi Andrew, can you please clarify the current status of the PR? Do you want me to help you with it? Namely i can resolve the conflicts with the fork of your branch - should i? |
Hi, I think it's just waiting on review. It seems to have gotten out of date in the meantime. |
* upstream/main: (33 commits) Fix virtual-env toolcache links Updated @actions/cache (actions#382) ci(workflow): add 'npm' cache for actions/setup-node in .github/workflows (actions#379) Cache hit output (actions#373) Add pyton-version to setup PyPy output (actions#365) Rework pipenv caching test (actions#375) Update README.md to fix setup-python version in example (actions#368) dist fix (actions#367) Cache on ghes (actions#363) Update dist Use `\n` instead of `os.EOL` Update dist Initialise pyproject.toml Build and format Remove console.log Remove unused file Reduce test matrix Parse values from poetry Release Add more tests ...
I've updated it with the latest upstream changes and rebuilt so there's no longer a merge conflict. Lmk if there's anything else. |
@@ -21,6 +22,27 @@ async function cacheDependencies(cache: string, pythonVersion: string) { | |||
await cacheDistributor.restoreCache(); | |||
} | |||
|
|||
function resolveVersionInput(): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think python-version input should take precedence over the python-version-file. It should be similar to this one https://github.com/actions/setup-node/blob/main/src/main.ts#L66-L97
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, yes, but as noted this would be a breaking change due to the YAML default of python-version: '3.x'
. This is a difference from setup-node. It means python-version
is always specified and it cannot be unspecified. So, by the setup-node logic, python-version-file
would never be used no matter what the user does.
If we wanted to remove the problematic default and require the user to specify at least one of version
or version-file
, like setup-node does, then the action would start crashing for existing setups who relied on that default. I would guess that's most of them.
I agree that setup-node makes more sense, and parity between the setup-* actions is desirable. I think to do that we'd have to bump major version and document the difference and config migration step. I'm not sure what the procedure is for such changes here, is that what you'd go with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. The action has default value for the python-version input, that is why it won't be empty string. I think we can rely on python-version-file with precedence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmitry-shibanov @adilosa I would say it might be better to introduce breaking change (and do a major release) to align with setup-node
and setup-ruby
actions.
With current solution setup-python
would end up in minority and potentially confuse users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means
python-version
is always specified
@adilosa i have a rather cumbersome idea how to make the python-version high priority without breaking the existing builds
- replace "default value" and for input with some string like "default" or "not-set"
- Handle that string by code, resolving to the the "3.x" python version
As a result:
- if you have python-version set to some meaningful value - use it as version despite of python-version-file
- if you have python-version set to the "default" value and python-version-file is not set - use "3.x"
- if you have python-version set to the "default" value and python-version-file is set - use python-version-file
Did not i miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it comes down to it, I'd be in favor of just doing a major version update and getting setup-python
in sync with the other setup-
actions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it comes down to it, I'd be in favor of just doing a major version update and getting
setup-python
in sync with the othersetup-
actions.
Hello @brcrista , so as we all seem to agree to make breaking change and bump major version, please confirm your approve, and either @adilosa or me will complete this piece of work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dsame, sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adilosa will you make the breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good - I've updated the PR with the new changes. it should be like the other setup-
actions now, which is nice.
Hello @adilosa. Could you please add unit tests for the |
.github/workflows/test-python.yml
Outdated
- name: Checkout | ||
uses: actions/checkout@v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be better to use action/checkout@v3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. This comes from the other tests in this file which all use v2, should I still update this one? or all of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of them, I believe
src/setup-python.ts
Outdated
function resolveVersionInput(): string { | ||
let version = core.getInput('python-version'); | ||
const versionFileInput = core.getInput('python-version-file'); | ||
|
||
if (versionFileInput) { | ||
const versionFilePath = path.join( | ||
process.env.GITHUB_WORKSPACE!, | ||
versionFileInput | ||
); | ||
if (!fs.existsSync(versionFilePath)) { | ||
throw new Error( | ||
`The specified node version file at: ${versionFilePath} does not exist` | ||
); | ||
} | ||
version = fs.readFileSync(versionFilePath, 'utf8'); | ||
core.info(`Resolved ${versionFileInput} as ${version}`); | ||
} | ||
|
||
return version; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to use a similar approach from setup-node
action? I mean, python-version
input has a higher priority than python-version-file
input.
@dmitry-shibanov, what do you think about it?
function resolveVersionInput(): string { | |
let version = core.getInput('python-version'); | |
const versionFileInput = core.getInput('python-version-file'); | |
if (versionFileInput) { | |
const versionFilePath = path.join( | |
process.env.GITHUB_WORKSPACE!, | |
versionFileInput | |
); | |
if (!fs.existsSync(versionFilePath)) { | |
throw new Error( | |
`The specified node version file at: ${versionFilePath} does not exist` | |
); | |
} | |
version = fs.readFileSync(versionFilePath, 'utf8'); | |
core.info(`Resolved ${versionFileInput} as ${version}`); | |
} | |
return version; | |
} | |
function resolveVersionInput(): string { | |
let version = core.getInput('python-version'); | |
const versionFileInput = core.getInput('python-version-file'); | |
if (version && versionFileInput) { | |
core.warning( | |
'Both python-version and python-version-file inputs are specified, only python-version will be used' | |
); | |
} | |
if (version) { | |
return version; | |
} | |
... | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dimitry mentioned the same, I replied in that thread :)
* python-version overrides python-version-file, like setup-node * checks '.python-version' by default if nothing else specified
Hi @adilosa, could you please revert the changes in README? It will be better to update the action's documentation after release of the new major version, because it can take some time after merging this PR. |
Yep, no problem. I've reverted the README change |
resolved the merge conflict. LGTM! |
Hi @adilosa, could you please update your branch from |
* upstream/main: Fix output for prerelease version of poetry (actions#409) Update zeit/ncc to vercel/ncc (actions#393) Change PyPy version to rebuild cache
@vsafonkin yep, should be updated now |
@adilosa, could you please rebuild javascript files? |
@vsafonkin updated. I had rerun the build already, but apparently I needed to update the installed dependencies as well. |
@adilosa, thank you for your contribution! We will release a new major version of |
Oh no! |
Description:
Support for a
python-version-file
input to read files such as.python-version
, similar to setup-node and setup-ruby.Since
python-version
has a default of '3.x' it is always specified - and altering that would be a breaking change - herepython-version-file
overridespython-version
. This is opposite to actions/setup-node.Related issue:
Fixes #291
Check list: