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.

Sign up for GitHub

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

Jump to bottom

add support for python-version-file #336

Merged
merged 14 commits into from Jun 2, 2022
Merged

add support for python-version-file #336

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5d2a353
add support for python-version-file
adilosa Feb 10, 2022
5f05e1d
Update action.yml
adilosa Mar 4, 2022
a200bff
Merge remote-tracking branch 'upstream/main'
adilosa Apr 19, 2022
dbfda6e
Merge branch 'actions:main' into main
adilosa May 17, 2022
abfae9a
update to v4, remove python-version default
adilosa May 17, 2022
e30cfd0
update tests, update to checkout@v3
adilosa May 17, 2022
bce43cf
update build
adilosa May 17, 2022
c38a07e
appease the linter
adilosa May 17, 2022
74e704b
remove old test for default python version
adilosa May 17, 2022
a6063fd
Merge branch 'actions:main' into main
adilosa May 20, 2022
0139695
revert readme changes
adilosa May 20, 2022
0669e36
Merge branch 'main' into main
adilosa May 26, 2022
59e9c1d
Merge remote-tracking branch 'upstream/main'
adilosa Jun 2, 2022
76b3fbd
update build
adilosa Jun 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 25 additions & 11 deletions .github/workflows/test-python.yml
View file
Open in desktop
Expand Up @@ -10,30 +10,41 @@ on:
- '**.md'
schedule:
- cron: 30 3 * * *
workflow_dispatch:

jobs:
default-version:
name: Setup default version
setup-versions-from-manifest:
name: Setup ${{ matrix.python }} ${{ matrix.os }}
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
uses: actions/checkout@v3

- name: setup default python
- name: setup-python ${{ matrix.python }}
uses: ./
with:
python-version: ${{ matrix.python }}

- name: Validate version
run: python --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 python code
- name: Run simple code
run: python -c 'import math; print(math.factorial(5))'

setup-versions-from-manifest:
name: Setup ${{ matrix.python }} ${{ matrix.os }}
setup-versions-from-file:
name: Setup ${{ matrix.python }} ${{ matrix.os }} version file
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
Expand All @@ -42,12 +53,15 @@ jobs:
python: [3.5.4, 3.6.7, 3.7.5, 3.8.1]
steps:
- name: Checkout
uses: actions/checkout@v2
uses: actions/checkout@v3

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

- name: setup-python ${{ matrix.python }}
uses: ./
with:
python-version: ${{ matrix.python }}
python-version-file: '.python-version'

- name: Validate version
run: |
Expand All @@ -71,7 +85,7 @@ jobs:
os: [macos-latest, windows-latest, ubuntu-18.04, ubuntu-20.04]
steps:
- name: Checkout
uses: actions/checkout@v2
uses: actions/checkout@v3

- name: setup-python 3.9.0-beta.4
uses: ./
Expand Down
5 changes: 3 additions & 2 deletions action.yml
View file
Open in desktop
Expand Up @@ -4,8 +4,9 @@ description: 'Set up a specific version of Python and add the command-line tools
author: 'GitHub'
inputs:
python-version:
description: "Version range or exact version of a Python version to use, using SemVer's version range syntax."
default: '3.x'
description: "Version range or exact version of Python to use, using SemVer's version range syntax. Reads from .python-version if unset."
python-version-file:
description: "File containing the Python version to use. Example: .python-version"
cache:
description: 'Used to specify a package manager for caching in the default directory. Supported values: pip, pipenv, poetry.'
required: false
Expand Down
23 changes: 22 additions & 1 deletion dist/setup/index.js
View file
Open in desktop
Expand Up @@ -6064,12 +6064,16 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, ge
step((generator = generator.apply(thisArg, _arguments || [])).next());
});
};
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__(847));
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 @@ -6082,6 +6086,23 @@ function cacheDependencies(cache, pythonVersion) {
yield cacheDistributor.restoreCache();
});
}
function resolveVersionInput() {
let version = core.getInput('python-version');
const versionFile = core.getInput('python-version-file');
if (version && versionFile) {
core.warning('Both python-version and python-version-file inputs are specified, only python-version will be used');
}
if (version) {
return version;
}
const versionFilePath = path.join(process.env.GITHUB_WORKSPACE, versionFile || '.python-version');
if (!fs_1.default.existsSync(versionFilePath)) {
throw new Error(`The specified python version file at: ${versionFilePath} does not exist`);
}
version = fs_1.default.readFileSync(versionFilePath, 'utf8');
core.info(`Resolved ${versionFile} as ${version}`);
return version;
}
function run() {
var _a;
return __awaiter(this, void 0, void 0, function* () {
Expand All @@ -6093,7 +6114,7 @@ function run() {
core.debug(`Python is expected to be installed into RUNNER_TOOL_CACHE==${process.env['RUNNER_TOOL_CACHE']}`);
}
try {
const version = core.getInput('python-version');
const version = resolveVersionInput();
if (version) {
let pythonVersion;
const arch = core.getInput('architecture') || os.arch();
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
@@ -1,6 +1,6 @@
{
"name": "setup-python",
"version": "3.1.1",
"version": "4.0.0",
"private": true,
"description": "Setup python action",
"main": "dist/index.js",
Expand Down
32 changes: 31 additions & 1 deletion src/setup-python.ts
View file
Open in desktop
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 {isCacheFeatureAvailable} from './utils';

Expand All @@ -21,6 +22,35 @@ 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.

Copy link

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 versionFile = core.getInput('python-version-file');

if (version && versionFile) {
core.warning(
'Both python-version and python-version-file inputs are specified, only python-version will be used'
);
}

if (version) {
return version;
}

const versionFilePath = path.join(
process.env.GITHUB_WORKSPACE!,
versionFile || '.python-version'
);
if (!fs.existsSync(versionFilePath)) {
throw new Error(
`The specified python version file at: ${versionFilePath} does not exist`
);
}
version = fs.readFileSync(versionFilePath, 'utf8');
core.info(`Resolved ${versionFile} as ${version}`);

return version;
}

async function run() {
if (process.env.AGENT_TOOLSDIRECTORY?.trim()) {
core.debug(
Expand All @@ -33,7 +63,7 @@ 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