-
Notifications
You must be signed in to change notification settings - Fork 520
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
Changes from 3 commits
5d2a353
5f05e1d
a200bff
dbfda6e
abfae9a
e30cfd0
bce43cf
c38a07e
74e704b
a6063fd
0139695
0669e36
59e9c1d
76b3fbd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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 commentThe 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 commentThe 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 If we wanted to remove the problematic default and require the user to specify at least one of 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 commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@adilosa i have a rather cumbersome idea how to make the python-version high priority without breaking the existing builds
As a result:
Did not i miss something? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe 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 commentThe 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 commentThe 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 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to use a similar approach from
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dimitry mentioned the same, I replied in that thread :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
async function run() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (process.env.AGENT_TOOLSDIRECTORY?.trim()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
core.debug( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -33,7 +55,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(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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