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

Use Python Version: let user select architecture as an advanced option #6937

Merged
merged 7 commits into from
Apr 11, 2018
Merged

Use Python Version: let user select architecture as an advanced option #6937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fd722ca
Bump minor version
brcrista Apr 10, 2018
8d4a7b9
add failing tests
brcrista Apr 10, 2018
34092dd
expose another bug
brcrista Apr 10, 2018
2642272
List versions with architecture when a match can't be found
brcrista Apr 10, 2018
da86097
Add "architecture" parameter under "Advanced"
brcrista Apr 10, 2018
6a2ad3b
Add "Advanced" tab
brcrista Apr 10, 2018
020e836
Architecture parameter is required
brcrista Apr 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Tasks/UsePythonVersion/Strings/resources.resjson/en-US/resources.resjson
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@
"loc.helpMarkDown": "[More Information](https://go.microsoft.com/fwlink/?linkid=871498)",
"loc.description": "Retrieves the specified version of Python from the tool cache. Optionally add it to PATH.",
"loc.instanceNameFormat": "Use Python $(versionSpec)",
"loc.group.displayName.advanced": "Advanced",
"loc.input.label.versionSpec": "Version spec",
"loc.input.help.versionSpec": "Version range or exact version of a Python version to use.",
"loc.input.label.addToPath": "Add to PATH",
"loc.input.help.addToPath": "Whether to prepend the retrieved Python version to the PATH environment variable to make it available in subsequent tasks or scripts without using the output variable.",
"loc.input.label.architecture": "Architecture",
"loc.input.help.architecture": "The target architecture (x86, x64) of the Python interpeter.",
"loc.messages.ListAvailableVersions": "Available versions:",
"loc.messages.PlatformNotRecognized": "Platform not recognized",
"loc.messages.PrependPath": "Prepending PATH environment variable with directory: %s",
Expand Down
57 changes: 49 additions & 8 deletions Tasks/UsePythonVersion/Tests/L0.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,15 +82,16 @@ describe('UsePythonVersion L0 Suite', function () {
};
mockery.registerMock('vsts-task-lib/task', Object.assign({}, mockTask, mockBuildVariables));

const toolPath = path.join('/', 'Python', '3.6.4');
const toolPath = path.join('/', 'Python', '3.6.4', 'x64');
mockery.registerMock('vsts-task-tool-lib/tool', {
findLocalTool: () => toolPath
});

const uut = reload();
const parameters = {
versionSpec: '3.6',
addToPath: false
addToPath: false,
architecture: 'x64'
};

assert.strictEqual(buildVariables['pythonLocation'], undefined);
Expand All @@ -103,13 +104,14 @@ describe('UsePythonVersion L0 Suite', function () {
mockery.registerMock('vsts-task-lib/task', mockTask);
mockery.registerMock('vsts-task-tool-lib/tool', {
findLocalTool: () => null,
findLocalToolVersions: () => ['2.7.13']
findLocalToolVersions: () => ['2.6.0', '2.7.13']
});

const uut = reload();
const parameters = {
versionSpec: '3.x',
addToPath: false
addToPath: false,
architecture: 'x64'
};

try {
Expand All @@ -119,14 +121,52 @@ describe('UsePythonVersion L0 Suite', function () {
const expectedMessage = [
'loc_mock_VersionNotFound 3.x',
'loc_mock_ListAvailableVersions',
'2.7.13'
'2.6.0 (x86)',
'2.7.13 (x86)',
'2.6.0 (x64)',
'2.7.13 (x64)'
].join(EOL);

assert.strictEqual(e.message, expectedMessage);
done();
}
});

it('selects architecture passed as input', async function () {
let buildVariables: { [key: string]: string } = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you can make this const.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I've come around to your opinion on this. I'll change this next time I touch this file.

const mockBuildVariables = {
setVariable: (variable: string, value: string) => {
buildVariables[variable] = value;
},
getVariable: (variable: string) => buildVariables[variable]
};
mockery.registerMock('vsts-task-lib/task', Object.assign({}, mockTask, mockBuildVariables));

const x86ToolPath = path.join('/', 'Python', '3.6.4', 'x86');
const x64ToolPath = path.join('/', 'Python', '3.6.4', 'x64');
mockery.registerMock('vsts-task-tool-lib/tool', {
findLocalTool: (toolName: string, versionSpec: string, arch?: string) => {
if (arch === 'x86') {
return x86ToolPath;
} else {
return x64ToolPath;
}
}
});

const uut = reload();
const parameters = {
versionSpec: '3.6',
addToPath: false,
architecture: 'x86'
};

assert.strictEqual(buildVariables['pythonLocation'], undefined);

await uut.usePythonVersion(parameters, Platform.Linux);
assert.strictEqual(buildVariables['pythonLocation'], x86ToolPath);
});

it('sets PATH correctly on Linux', async function () {
mockery.registerMock('vsts-task-lib/task', mockTask);

Expand All @@ -145,8 +185,8 @@ describe('UsePythonVersion L0 Suite', function () {
const uut = reload();
const parameters = {
versionSpec: '3.6',
outputVariable: 'Python',
addToPath: true
addToPath: true,
architecture: 'x64'
};

await uut.usePythonVersion(parameters, Platform.Linux);
Expand All @@ -173,7 +213,8 @@ describe('UsePythonVersion L0 Suite', function () {
const uut = reload();
const parameters = {
versionSpec: '3.6',
addToPath: true
addToPath: true,
architecture: 'x64'
};

await uut.usePythonVersion(parameters, Platform.Windows);
Expand Down
3 changes: 2 additions & 1 deletion Tasks/UsePythonVersion/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import { usePythonVersion } from './usepythonversion';
task.setResourcePath(path.join(__dirname, 'task.json'));
await usePythonVersion({
versionSpec: task.getInput('versionSpec', true),
addToPath: task.getBoolInput('addToPath', true)
addToPath: task.getBoolInput('addToPath', true),
architecture: task.getInput('architecture', true)
},
getPlatform());
task.setResult(task.TaskResult.Succeeded, "");
Expand Down
24 changes: 22 additions & 2 deletions Tasks/UsePythonVersion/task.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,19 @@
"author": "Microsoft Corporation",
"version": {
"Major": 0,
"Minor": 133,
"Patch": 2
"Minor": 134,
"Patch": 0
},
"preview": true,
"demands": [],
"instanceNameFormat": "Use Python $(versionSpec)",
"groups": [
{
"name": "advanced",
"displayName": "Advanced",
"isExpanded": false
}
],
"inputs": [
{
"name": "versionSpec",
Expand All @@ -34,6 +41,19 @@
"required": true,
"defaultValue": "true",
"helpMarkDown": "Whether to prepend the retrieved Python version to the PATH environment variable to make it available in subsequent tasks or scripts without using the output variable."
},
{
"name": "architecture",
"type": "pickList",
"label": "Architecture",
"defaultValue": "x64",
"required": true,
"helpMarkDown": "The target architecture (x86, x64) of the Python interpeter.",
"groupName": "advanced",
"options": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this is appropriate here, but in most cases with options like this we have a "default" option that is selected automatically and allows the Task to make the most appropriate choice. I.e. choose the one that matches the OS arch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See a few lines above, the default is x64. The agent's architecture won't be known at build definition time, but the hosted agents only have x64 Pythons.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have the "default" on several picklists (e.g. Java version) to make it just work on any agent but it does cause a lot of confusion. In this case not having "default" is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpricketMSFT Sorry, I think I misunderstood you the first time. Sounds like you were talking about "passing null" to the task and letting the task choose at runtime. But I think the best experience is to pass 'x64' unless the user says otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is basically what I was thinking, but as Madhuri says, it can cause a lot of confusion. This is better.

"x86": "x86",
"x64": "x64"
}
}
],
"outputVariables": [
Expand Down
24 changes: 22 additions & 2 deletions Tasks/UsePythonVersion/task.loc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,19 @@
"author": "Microsoft Corporation",
"version": {
"Major": 0,
"Minor": 133,
"Patch": 2
"Minor": 134,
"Patch": 0
},
"preview": true,
"demands": [],
"instanceNameFormat": "ms-resource:loc.instanceNameFormat",
"groups": [
{
"name": "advanced",
"displayName": "ms-resource:loc.group.displayName.advanced",
"isExpanded": false
}
],
"inputs": [
{
"name": "versionSpec",
Expand All @@ -34,6 +41,19 @@
"required": true,
"defaultValue": "true",
"helpMarkDown": "ms-resource:loc.input.help.addToPath"
},
{
"name": "architecture",
"type": "pickList",
"label": "ms-resource:loc.input.label.architecture",
"defaultValue": "x64",
"required": true,
"helpMarkDown": "ms-resource:loc.input.help.architecture",
"groupName": "advanced",
"options": {
"x86": "x86",
"x64": "x64"
}
}
],
"outputVariables": [
Expand Down
20 changes: 15 additions & 5 deletions Tasks/UsePythonVersion/usepythonversion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,29 +11,39 @@ import { Platform } from './taskutil';
import * as toolUtil from './toolutil';

interface TaskParameters {
readonly versionSpec: string,
readonly addToPath: boolean
versionSpec: string,
addToPath: boolean,
architecture: string
}

export function pythonVersionToSemantic(versionSpec: string) {
const prereleaseVersion = /(\d+\.\d+\.\d+)([a|b|rc]\d*)/g;
return versionSpec.replace(prereleaseVersion, '$1-$2');
}

export async function usePythonVersion(parameters: TaskParameters, platform: Platform): Promise<void> {
export async function usePythonVersion(parameters: Readonly<TaskParameters>, platform: Platform): Promise<void> {
// Python's prelease versions look like `3.7.0b2`.
// This is the one part of Python versioning that does not look like semantic versioning, which specifies `3.7.0-b2`.
// If the version spec contains prerelease versions, we need to convert them to the semantic version equivalent
const semanticVersionSpec = pythonVersionToSemantic(parameters.versionSpec);
task.debug(`Semantic version spec of ${parameters.versionSpec} is ${semanticVersionSpec}`);

const installDir: string | null = tool.findLocalTool('Python', semanticVersionSpec);
const installDir: string | null = tool.findLocalTool('Python', semanticVersionSpec, parameters.architecture);
if (!installDir) {
// Fail and list available versions
const x86Versions = tool.findLocalToolVersions('Python', 'x86')
.map(s => `${s} (x86)`)
.join(os.EOL);

const x64Versions = tool.findLocalToolVersions('Python', 'x64')
.map(s => `${s} (x64)`)
.join(os.EOL);

throw new Error([
task.loc('VersionNotFound', parameters.versionSpec),
task.loc('ListAvailableVersions'),
tool.findLocalToolVersions('Python')
x86Versions,
x64Versions
].join(os.EOL));
}

Expand Down