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

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

Merged
merged 7 commits into from
Apr 11, 2018

Conversation

brcrista
Copy link
Contributor

@brcrista brcrista commented Apr 10, 2018

The hosted agents only have 64-bit versions of Python installed, but users are able to install 32-bit versions on private agents. They can also install 32-bit versions on hosted Windows agents from NuGet using the 3rd party "Install Python" task.

Since hosted agents only have 64-bit, I am putting this setting under the "Advanced" tab with a default value of x64. When the task fails to find a version, it will now list versions with architectures available to the agent.

Testing:

  • L0
  • Manually with private Windows agent

@brcrista brcrista changed the title Let user select architecture as an advanced option Use Python Version: let user select architecture as an advanced option Apr 10, 2018
].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.

"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.

Copy link
Member

@jpricket jpricket left a comment

Choose a reason for hiding this comment

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

Looks good. one comment

@brcrista brcrista merged commit b2c4065 into master Apr 11, 2018
@brcrista brcrista deleted the users/brcrista/use-version-architecture branch April 13, 2018 15:02
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.

4 participants