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 non-stable SDK version for SB artifacts #44362

Conversation

NikolaMilosavljevic
Copy link
Member

Fixes: dotnet/source-build#4682

Due to several changes made early in 9.0, particularly dotnet/installer#18153, we're versioning GA and Servicing source-build packages with SDK's stable version. This was a result of a switch to not use the version from the intermediate package, which was always of non-stable kind. We're now using the version in sdk tarballs filename, which is always stable for GA and during servicing.

The fix is to obtain the non-stable version from SDK's .version file.

Currently, in failing 9.0 VMR build:

dotnet-sdk-9.0.100-fedora.40-x64.tar.gz
dotnet-symbols-all-9.0.100-fedora.40-x64.tar.gz
dotnet-symbols-sdk-9.0.100-fedora.40-x64.tar.gz
Private.SourceBuilt.Artifacts.9.0.100.fedora.40-x64.tar.gz

With this fix:

dotnet-sdk-9.0.100-fedora.40-x64.tar.gz
dotnet-symbols-all-9.0.100-rtm.24521.1-fedora.40-x64.tar.gz
dotnet-symbols-sdk-9.0.100-rtm.24521.1-fedora.40-x64.tar.gz
Private.SourceBuilt.Artifacts.9.0.100-rtm.24521.1.fedora.40-x64.tar.gz

This also updates the content of the .version file in Private.SourceBuilt.Artifacts.* package, to use the non-stable version. This matches the expected experience from previous releases, i.e. 8.0 GA.

8.0 RTM and GA packages, for comparison, can be obtained from links in following PRs: dotnet/installer#17784, dotnet/installer#20172

@NikolaMilosavljevic NikolaMilosavljevic requested review from a team as code owners October 22, 2024 15:57
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Oct 22, 2024
Comment on lines 33 to 44
<UsingTask TaskName="IndexItemGroup" TaskFactory="RoslynCodeTaskFactory" AssemblyFile="$(MSBuildToolsPath)\Microsoft.Build.Tasks.Core.dll" >
<ParameterGroup>
<Items Required="true" ParameterType="Microsoft.Build.Framework.ITaskItem[]"/>
<Index Required="true" ParameterType="System.Int32"/>
<Item Output="true" ParameterType="Microsoft.Build.Framework.ITaskItem"/>
</ParameterGroup>
<Task>
<Code Type="Fragment" Language="cs">
<![CDATA[Item = Items[ Index ];]]>
</Code>
</Task>
</UsingTask>
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be our first inline C# msbuild task. Could we just a regex pattern below which is already natively supported as an msbuild function?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I see that you just want to read a specific line. I think that should already be supported in msbuild without a task. cc @rainersigwald

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

We do use inline msbuild tasks in other places, in VMR. But, I like the idea of avoiding the task if possible - will check the other suggested solution.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. Are you sure that the VMR uses inline tasks? I can't find any hits: https://github.com/search?q=repo%3Adotnet%2Fsdk%20RoslynCodeTaskFactory&type=code

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed with 2c1e9e2

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

OK I thought you meant our infra that we own, the VMR orchestrator, That one is a workaround thing that will be removed again in ~ a month.

<Output PropertyName="SourceBuiltSdkNonStableVersion" TaskParameter="Item"/>
</IndexItemGroup>
<PropertyGroup>
<SourceBuiltSdkNonStableVersion>$([System.Text.RegularExpressions.Regex]::Split('$([System.IO.File]::ReadAllText('%(SdkVersionFileItem.Identity)'))', '\r\n|\r|\n')[3])</SourceBuiltSdkNonStableVersion>
Copy link
Member

Choose a reason for hiding this comment

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

Can ReadAllLines be used instead to simplify the logic?

Copy link
Member Author

Choose a reason for hiding this comment

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

I could not get ReadAllLines to work in MSBuild: error MSB4185: The function "ReadAllLines" on type "System.IO.File" is not available for execution as an MSBuild property function

There is a built-in task as an alternative - ReadLinesFromFile. But, then the code gets a bit larger.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah not sure why we allow ReadAllText but not ReadAllLines; I'd be willing to expand that if you care to file it. But this seems ok (or using string.Split()).

@NikolaMilosavljevic
Copy link
Member Author

Validation build was successful.

@NikolaMilosavljevic NikolaMilosavljevic merged commit 02baa35 into dotnet:release/9.0.1xx Oct 22, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants