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

[macOS] Return the correct minor version on 11 and later OS. #3605

Merged
merged 11 commits into from
Jan 17, 2022

Conversation

mandel-macaque
Copy link
Member

The OS returned from 11.0 and later is:

11.* = 20.6.0.0 (for 11.6)
12.* - 21.2.0.0 (for 12.2 beta)

The minor in those versions is correct. This fix ensures that Agent
capabilities can match the minor version too.

The OS returned from 11.0 and later is:

11.* = 20.6.0.0
12.* - 21.2.0.0

The minor in those versions is correct. This fix ensures that Agent
capabilities can match the minor version too.
mandel-macaque added a commit to mandel-macaque/xamarin-macios that referenced this pull request Nov 10, 2021
The agent is not reporting the OS version property and returns only
11.0. This has been fixed in:

microsoft/azure-pipelines-agent#3605

But this was not yet landed.
@@ -117,7 +117,7 @@ private static string GetDarwinVersionString()
}
else
{
return $"11.{version.Major - 20}";
return $"11.{version.Minor}";
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like this method is ever going to return 12.*, or am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Very good point, fixing that

mandel-macaque added a commit to xamarin/xamarin-macios that referenced this pull request Nov 10, 2021
The agent is not reporting the OS version property and returns only
11.0. This has been fixed in:

microsoft/azure-pipelines-agent#3605

But this was not yet landed.
@mandel-macaque
Copy link
Member Author

/cc @fadnavistanmay

@alexander-smolyakov
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mandel-macaque
Copy link
Member Author

mandel-macaque commented Dec 14, 2021

@alexander-smolyakov can we land this?

@nycnewman
Copy link

This should also fix #3655

@alexander-smolyakov
Copy link
Contributor

/azp run

@alexander-smolyakov alexander-smolyakov self-assigned this Dec 29, 2021
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@alexander-smolyakov
Copy link
Contributor

@alexander-smolyakov can we land this?

@mandel-macaque, sorry for the delay on this one, our team is currently on holiday until next week. We are planning to finish the testing of these changes and merge them next week.

@alexander-smolyakov
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -117,7 +117,7 @@ private static string GetDarwinVersionString()
}
else
{
return $"11.{version.Major - 20}";
return $"{version.Major - 9}.{version.Minor}";
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be version.Minor - 1 if version.Minor > 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

No need, minor version numbers start at 0, not 1. Running on Monterey you get:

Enter statements below.
csharp> var version = Environment.OSVersion.Version;
csharp> version.Minor
2
csharp> version
21.2.0.0
csharp>

Choose a reason for hiding this comment

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

Which version of Monterey were you running? If this was 12.1.0 then seems you need to decrease version by one. i.e. 21.2.0 -> 12.1 (which is on the machine I am running now)

Copy link
Member Author

Choose a reason for hiding this comment

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

ugh apple.. yes you are right, fixing it.

Copy link
Contributor

@anatolybolshakov anatolybolshakov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for contribution!

@alexander-smolyakov
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dalexsoto
Copy link
Member

dalexsoto commented Jan 15, 2022

Hello @alexander-smolyakov / anatolybolshakov anything else needed to get this merged?

@alexander-smolyakov
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@alexander-smolyakov
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@anatolybolshakov
Copy link
Contributor

@mandel-macaque thanks! We've merged the PR and going to roll out these changes with the next agent release.

@alexander-smolyakov
Copy link
Contributor

Quick Update: Agent v2.198.3, which contains the related changes, has been fully rolled out.

cc @mandel-macaque @nycnewman

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants