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

add support for python-version-file #336

Merged
merged 14 commits into from Jun 2, 2022
Merged

add support for python-version-file #336

merged 14 commits into from Jun 2, 2022

Conversation

adilosa
Copy link
Contributor

@adilosa adilosa commented Feb 10, 2022

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 - here python-version-file overrides python-version. This is opposite to actions/setup-node.

Related issue:
Fixes #291

Check list:

  • Mark if documentation changes are required. (README was updated)
  • Mark if tests were added or updated to cover the changes.

Copy link

@lukasz-mitka lukasz-mitka left a 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.

.github/workflows/test-python.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
@adilosa
Copy link
Contributor Author

adilosa commented Mar 1, 2022

I think the presence of the python-version-file is enough of a signal to use the file, without also having to set an additional magic value to python-version. This also keeps it more consistent with the other actions/setup-node and actions/setup-ruby actions.

@lukasz-mitka
Copy link

I think the presence of the python-version-file is enough of a signal to use the file, without also having to set an additional magic value to python-version. This also keeps it more consistent with the other actions/setup-node and actions/setup-ruby actions.

Ok, I see what you are trying to achieve.

actions/setup-node uses conflict resolution which is missing here https://github.com/actions/setup-node/blob/main/src/main.ts#L80
Also python-version-file should have description that it's inferior to python-version.

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

@adilosa
Copy link
Contributor Author

adilosa commented Mar 4, 2022

actions/setup-node uses conflict resolution which is missing here https://github.com/actions/setup-node/blob/main/src/main.ts#L80

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 python-version has a default of '3.x' specified. This means the variable is literally always set and cannot be unset, so a conflict would always happen when python-version-file is specified. Not a huge fan of this, but as stated in the PR comment altering it would be a breaking change.

Also python-version-file should have description that it's inferior to python-version.

Yep, agreed. I'll update that.

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

Also agreed. If it weren't for python-version having a default specified, this would make more sense IMO and be more consistent. Unfortunately will have to be another PR and probably major version increase I suspect.

@dsame
Copy link
Contributor

dsame commented Apr 19, 2022

@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?

@adilosa
Copy link
Contributor Author

adilosa commented Apr 19, 2022

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
  ...
@adilosa
Copy link
Contributor Author

adilosa commented Apr 19, 2022

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 {
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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.

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.

Copy link
Contributor

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

  1. replace "default value" and for input with some string like "default" or "not-set"
  2. 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?

Copy link
Contributor

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.

Copy link
Contributor

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.

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.

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

@dmitry-shibanov
Copy link
Contributor

Hello @adilosa. Could you please add unit tests for the python-version-file input.

Comment on lines 75 to 76
- name: Checkout
uses: actions/checkout@v2

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

Copy link
Contributor Author

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?

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

Comment on lines 25 to 44
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;
}
Copy link

@vsafonkin vsafonkin Apr 20, 2022

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?

Suggested change
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;
}
...
}

Copy link
Contributor Author

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 :)

#336 (comment)

@vsafonkin
Copy link

vsafonkin commented May 18, 2022

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.

@adilosa
Copy link
Contributor Author

adilosa commented May 20, 2022

Yep, no problem. I've reverted the README change

@adilosa
Copy link
Contributor Author

adilosa commented May 26, 2022

resolved the merge conflict. LGTM!

@vsafonkin
Copy link

Hi @adilosa, could you please update your branch from main to fix poetry cache test?

* 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
@adilosa
Copy link
Contributor Author

adilosa commented Jun 2, 2022

@vsafonkin yep, should be updated now

@vsafonkin
Copy link

@adilosa, could you please rebuild javascript files?

@adilosa
Copy link
Contributor Author

adilosa commented Jun 2, 2022

@vsafonkin updated. I had rerun the build already, but apparently I needed to update the installed dependencies as well.

@vsafonkin vsafonkin merged commit 53e1529 into actions:main Jun 2, 2022
@vsafonkin
Copy link

@adilosa, thank you for your contribution! We will release a new major version of setup-python action next week.

@plazum
Copy link

plazum commented Jun 13, 2022

Oh no!
I'm sad.
I indeed rely on the default 3.x. And you killed it. And all the workflow runs after I updated the use of this action fail.
Well, I know it's my fault for not reading the change logs or something.
But I still think it's good to keep a default selection, so you can use this action in an elegant way.

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.

Support a .python-version file
8 participants