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
34 changes: 34 additions & 0 deletions .github/workflows/test-python.yml
Expand Up @@ -10,6 +10,7 @@ on:
- '**.md'
schedule:
- cron: 30 3 * * *
workflow_dispatch:

jobs:
default-version:
Expand Down Expand Up @@ -62,6 +63,39 @@ jobs:
- name: Run simple code
run: python -c 'import math; print(math.factorial(5))'

setup-versions-from-file:
name: Setup ${{ matrix.python }} ${{ matrix.os }} version file
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
os: [macos-latest, windows-latest, ubuntu-18.04, ubuntu-20.04]
python: [3.5.4, 3.6.7, 3.7.5, 3.8.1]
steps:
- 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


- name: build-version-file ${{ matrix.python }}
run: echo ${{ matrix.python }} > .python-version

- name: setup-python ${{ matrix.python }}
uses: ./
with:
python-version-file: '.python-version'
adilosa marked this conversation as resolved.
Show resolved Hide resolved

- name: Validate version
run: |
$pythonVersion = (python --version)
if ("Python ${{ matrix.python }}" -ne "$pythonVersion"){
Write-Host "The current version is $pythonVersion; expected version is ${{ matrix.python }}"
exit 1
}
$pythonVersion
shell: pwsh

- name: Run simple code
run: python -c 'import math; print(math.factorial(5))'

setup-pre-release-version-from-manifest:
name: Setup 3.9.0-beta.4 ${{ matrix.os }}
runs-on: ${{ matrix.os }}
Expand Down
12 changes: 12 additions & 0 deletions README.md
Expand Up @@ -20,6 +20,7 @@ This action sets up a Python environment for use in actions by:
- Support for pre-release versions of Python.
- Support for installing any version of PyPy on-flight
- Support for built-in caching of pip and pipenv dependencies
- Support for `.python-version` file

# Usage

Expand All @@ -36,6 +37,17 @@ steps:
- run: python my_script.py
```

Read Python version from file:
```yaml
steps:
- uses: actions/checkout@v2
- uses: actions/setup-python@v2
with:
python-version-file: '.python-version' # Read python version from a file
architecture: 'x64' # optional x64 or x86. Defaults to x64 if not specified
- run: python my_script.py
```

Matrix Testing:
```yaml
jobs:
Expand Down
2 changes: 2 additions & 0 deletions action.yml
Expand Up @@ -6,6 +6,8 @@ inputs:
python-version:
description: "Version range or exact version of a Python version to use, using SemVer's version range syntax."
default: '3.x'
python-version-file:
description: "File containing the Python version to use. Examples: .python-version"
adilosa marked this conversation as resolved.
Show resolved Hide resolved
cache:
description: 'Used to specify a package manager for caching in the default directory. Supported values: pip, pipenv.'
required: false
Expand Down
19 changes: 18 additions & 1 deletion dist/setup/index.js
Expand Up @@ -6622,12 +6622,16 @@ var __importStar = (this && this.__importStar) || function (mod) {
result["default"] = mod;
return result;
};
var __importDefault = (this && this.__importDefault) || function (mod) {
return (mod && mod.__esModule) ? mod : { "default": mod };
};
Object.defineProperty(exports, "__esModule", { value: true });
const core = __importStar(__webpack_require__(470));
const finder = __importStar(__webpack_require__(927));
const finderPyPy = __importStar(__webpack_require__(50));
const path = __importStar(__webpack_require__(622));
const os = __importStar(__webpack_require__(87));
const fs_1 = __importDefault(__webpack_require__(747));
const cache_factory_1 = __webpack_require__(633);
const utils_1 = __webpack_require__(163);
function isPyPyVersion(versionSpec) {
Expand All @@ -6643,10 +6647,23 @@ function cacheDependencies(cache, pythonVersion) {
yield cacheDistributor.restoreCache();
});
}
function resolveVersionInput() {
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_1.default.existsSync(versionFilePath)) {
throw new Error(`The specified node version file at: ${versionFilePath} does not exist`);
}
version = fs_1.default.readFileSync(versionFilePath, 'utf8');
core.info(`Resolved ${versionFileInput} as ${version}`);
}
return version;
}
function run() {
return __awaiter(this, void 0, void 0, function* () {
try {
const version = core.getInput('python-version');
const version = resolveVersionInput();
if (version) {
let pythonVersion;
const arch = core.getInput('architecture') || os.arch();
Expand Down
24 changes: 23 additions & 1 deletion src/setup-python.ts
Expand Up @@ -3,6 +3,7 @@ import * as finder from './find-python';
import * as finderPyPy from './find-pypy';
import * as path from 'path';
import * as os from 'os';
import fs from 'fs';
import {getCacheDistributor} from './cache-distributions/cache-factory';
import {isGhes} from './utils';

Expand All @@ -24,9 +25,30 @@ 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.

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)


async function run() {
try {
const version = core.getInput('python-version');
const version = resolveVersionInput();
if (version) {
let pythonVersion: string;
const arch: string = core.getInput('architecture') || os.arch();
Expand Down