Skip to content

Fix .NET Version Parsing for FindPath API #2000

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

Merged
merged 8 commits into from
Oct 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ suite('DotnetCoreAcquisitionExtension End to End', function()
if(shouldFind)
{
assert.exists(result.dotnetPath, 'find path command returned a result');
assert.equal(result.dotnetPath, installPath, 'The path returned by findPath is correct');
assert.equal(result.dotnetPath.toLowerCase(), installPath.toLowerCase(), 'The path returned by findPath is correct');
}
else
{
Expand Down Expand Up @@ -316,13 +316,28 @@ suite('DotnetCoreAcquisitionExtension End to End', function()
}
}).timeout(standardTimeoutTime);

test('Find dotnet PATH Command Unmet Version Condition', async () => {
// Install 3.1, look for 8.0 which is not less than or equal to 3.1
await findPathWithRequirementAndInstall('8.0', 'runtime', os.arch(), 'less_than_or_equal', false,
test('Find dotnet PATH Command Met Version Condition', async () => {
// Install 8.0, look for 3.1 with accepting dotnet gr than or eq to 3.1

await findPathWithRequirementAndInstall('8.0', 'runtime', os.arch(), 'greater_than_or_equal', true,
{version : '3.1', mode : 'runtime', architecture : os.arch(), requestingExtensionId : requestingExtensionId}
);
}).timeout(standardTimeoutTime);

test('Find dotnet PATH Command Met Version Condition with Double Digit Major', async () => {
await findPathWithRequirementAndInstall('9.0', 'runtime', os.arch(), 'less_than_or_equal', true,
{version : '11.0', mode : 'runtime', architecture : os.arch(), requestingExtensionId : requestingExtensionId}
);
}).timeout(standardTimeoutTime);


test('Find dotnet PATH Command Unmet Version Condition', async () => {
// Install 9.0, look for 90.0 which is not equal to 9.0
await findPathWithRequirementAndInstall('9.0', 'runtime', os.arch(), 'equal', false,
{version : '90.0', mode : 'runtime', architecture : os.arch(), requestingExtensionId : requestingExtensionId}
);
}).timeout(standardTimeoutTime);

test('Find dotnet PATH Command Unmet Mode Condition', async () => {
// look for 3.1 runtime but install 3.1 aspnetcore
await findPathWithRequirementAndInstall('3.1', 'runtime', os.arch(), 'equal', false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ export class DotnetConditionValidator implements IDotnetConditionValidator

if(availableRuntimes.some((runtime) =>
{
const foundVersion = versionUtils.getMajorMinor(runtime.version, this.workerContext.eventStream, this.workerContext);
const availableVersion = versionUtils.getMajorMinor(runtime.version, this.workerContext.eventStream, this.workerContext);
return runtime.mode === requirement.acquireContext.mode && this.stringArchitectureMeetsRequirement(hostArch, requirement.acquireContext.architecture) &&
this.stringVersionMeetsRequirement(foundVersion, requestedMajorMinor, requirement.versionSpecRequirement);
this.stringVersionMeetsRequirement(availableVersion, requestedMajorMinor, requirement.versionSpecRequirement);
}))
{
return true;
Expand All @@ -44,8 +44,8 @@ export class DotnetConditionValidator implements IDotnetConditionValidator
if(availableSDKs.some((sdk) =>
{
// The SDK includes the Runtime, ASP.NET Core Runtime, and Windows Desktop Runtime. So, we don't need to check the mode.
const foundVersion = versionUtils.getMajorMinor(sdk.version, this.workerContext.eventStream, this.workerContext);
return this.stringArchitectureMeetsRequirement(hostArch, requirement.acquireContext.architecture) && this.stringVersionMeetsRequirement(foundVersion, requestedMajorMinor, requirement.versionSpecRequirement);
const availableVersion = versionUtils.getMajorMinor(sdk.version, this.workerContext.eventStream, this.workerContext);
return this.stringArchitectureMeetsRequirement(hostArch, requirement.acquireContext.architecture) && this.stringVersionMeetsRequirement(availableVersion, requestedMajorMinor, requirement.versionSpecRequirement);
}))
{
return true;
Expand Down Expand Up @@ -136,22 +136,38 @@ Please set the PATH to a dotnet host that matches the architecture ${requirement
return os.platform() === 'win32' ? (await this.executor!.tryFindWorkingCommand([CommandExecutor.makeCommand('chcp', ['65001'])])) !== null : false;
}

private stringVersionMeetsRequirement(foundVersion : string, requiredVersion : string, requirement : DotnetVersionSpecRequirement) : boolean
private stringVersionMeetsRequirement(availableVersion : string, requestedVersion : string, requirement : DotnetVersionSpecRequirement) : boolean
{

Choose a reason for hiding this comment

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

can you use semver for comparison?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good suggestion, semver is a good option. I hadn't considered that, thank you. I wish we were using semver in the code a lot earlier in many places.

Going forward I will use that. I do want to get this merged quickly though, so I'm not sure about making that change now considering this is already written and tested 🤔

Choose a reason for hiding this comment

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

Can you file an issue to use semver rather than parsing versions ourselves?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good idea 👀 #2003

if(requirement === 'equal')
{
return foundVersion === requiredVersion;
}
else if(requirement === 'greater_than_or_equal')
const availableMajor = Number(versionUtils.getMajor(availableVersion, this.workerContext.eventStream, this.workerContext));
const requestedMajor = Number(versionUtils.getMajor(requestedVersion, this.workerContext.eventStream, this.workerContext));

if(availableMajor === requestedMajor)
{
return foundVersion >= requiredVersion;
const availableMinor = Number(versionUtils.getMinor(availableVersion, this.workerContext.eventStream, this.workerContext));
const requestedMinor = Number(versionUtils.getMinor(requestedVersion, this.workerContext.eventStream, this.workerContext));

switch(requirement)
{
case 'equal':
return availableMinor === requestedMinor;
case 'greater_than_or_equal':
return availableMinor >= requestedMinor;
case 'less_than_or_equal':
return availableMinor <= requestedMinor;
}
}
else if(requirement === 'less_than_or_equal')
else
{
return foundVersion <= requiredVersion;
switch(requirement)
{
case 'equal':
return false;
case 'greater_than_or_equal':
return availableMajor >= requestedMajor;
case 'less_than_or_equal':
return availableMajor <= requestedMajor;
}
}

return false;
}

private stringArchitectureMeetsRequirement(outputArchitecture : string, requiredArchitecture : string | null | undefined) : boolean
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,26 @@ const invalidFeatureBandErrorString = `A feature band couldn't be determined for
/**
*
* @param fullySpecifiedVersion the fully specified version of the sdk, e.g. 7.0.301 to get the major from.
* @returns the major.minor in the form of '3', etc.
* @returns the major in the form of '3', etc.
*/
export function getMajor(fullySpecifiedVersion : string, eventStream : IEventStream, context : IAcquisitionWorkerContext) : string
{
// The called function will check that we can do the split, so we don't need to check again.
return getMajorMinor(fullySpecifiedVersion, eventStream, context).split('.')[0];
}

/**
*
* @param fullySpecifiedVersion the fully specified version of the sdk, e.g. 7.0.301 to get the minor from.
* @returns the major.minor in the form of '0', etc.
*/
export function getMinor(fullySpecifiedVersion : string, eventStream : IEventStream, context : IAcquisitionWorkerContext) : string
{
// The called function will check that we can do the split, so we don't need to check again.
return getMajorMinor(fullySpecifiedVersion, eventStream, context).split('.')[1];
}


/**
*
* @param fullySpecifiedVersion the fully specified version, e.g. 7.0.301 to get the major minor from.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@
* Licensed to the .NET Foundation under one or more agreements.
* The .NET Foundation licenses this file to you under the MIT license.
*--------------------------------------------------------------------------------------------*/

/**
* @remarks A condition to be met when searching for .NET. This refers to the major.minor of .NET versions.
* When this condition is used, the available version is compared to the required version.
* For example, if the request is made looking for 8.0 and allowing 'greater_than_or_equal', then 10.0 would be accepted,
* because 10.0 >= 8.0.
*/
export type DotnetVersionSpecRequirement = 'equal' | 'greater_than_or_equal' | 'less_than_or_equal';

Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ suite('Version Utilities Unit Tests', () => {
assert.equal(resolver.getMajor(twoDigitMajorVersion, mockEventStream, mockCtx), '10');
});

test('Get Minor from SDK Version', async () => {
assert.equal(resolver.getMinor(fullySpecifiedVersion, mockEventStream, mockCtx), '0');
assert.equal(resolver.getMinor(uniqueMajorMinorVersion, mockEventStream, mockCtx), '1');
assert.equal(resolver.getMinor(twoDigitMajorVersion, mockEventStream, mockCtx), '0');
});

test('Get Major.Minor from SDK Version', async () => {
assert.equal(resolver.getMajorMinor(fullySpecifiedVersion, mockEventStream, mockCtx), '7.0');
assert.equal(resolver.getMajorMinor(featureBandVersion, mockEventStream, mockCtx), '7.0');
Expand Down