Skip to content

Conversation

@carlossanlop
Copy link
Contributor

@carlossanlop carlossanlop commented Apr 3, 2025

Draft.

Problems to solve:

  • [Fixed] Can't use either [xml]$csproj = Get-Content $projectFile or $csproj = New-Object System.Xml.XmlDocument because simple elements get auto-converted to strings by PowerShell and there's no power in this planet to delete them and convert them back to XmlElements. What worked was using Add-Type -AssemblyName System.Xml.Linq; $csproj = [System.Xml.Linq.XDocument]::Load($CsprojPath).
  • [Fixed] Now the problem with System.Xml.Linq is that the csprojs get completely reformatted, spaces removed, comments moved to newlines, etc.
  • [Fixed] The versioning properties for System.Runtime.CompilerServices.Unsafe are located in Versioning.props, not in the *proj file, and I can't treat it as a ProjectReference or the powershell script fails.
  • [Won't fix] The function that bumps the version is very naive. It only bumps the number component I tell it to bump. This is good for now. We can make it more complicated if needed.
  • Still need to figure out how to add this to the yaml workloads so it gets done automatically and a new PR is submitted by the bot.

@carlossanlop carlossanlop self-assigned this Apr 3, 2025
@ericstj
Copy link
Member

ericstj commented Apr 3, 2025

If it helps you can have a look at https://github.com/Azure/azure-sdk-for-net/blob/0076189829cb2e2c3139a64e221f0f53105b63e5/eng/scripts/Update-PkgVersion.ps1 which is what AzureSDK uses for the same task. We'll probably have our own needs based on style/conventions but it might have some interesting features/edge-cases to borrow.

@carlossanlop
Copy link
Contributor Author

If it helps you can have a look at https://github.com/Azure/azure-sdk-for-net/blob/0076189829cb2e2c3139a64e221f0f53105b63e5/eng/scripts/Update-PkgVersion.ps1 which is what AzureSDK uses for the same task. We'll probably have our own needs based on style/conventions but it might have some interesting features/edge-cases to borrow.

That's exactly where I got inspiration from.

I was initially considering adopting their functionality and modifying it to match our needs, but I realized it is extremely complex and does a lot of things we most likely don't need.

So instead I opted for keeping it as simple as possible, and increase complexity only where it's needed.

For example, I'm wondering if I even need to have msbuild tasks at all, since I could easily do everything with powershell.

@carlossanlop
Copy link
Contributor Author

Alright I reverted the msbuild changes, I'm doing everything with powershell now, it's much simpler and straightforward.

I added a couple of commits that tested the script and it worked. I then reverted them because the endlines are getting changed, I don't want those changes merged.

Now I need to work on the yaml.

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

Some of these comments were from before your latest changes, but leaving them in just to give perspective on target vs xml approach.

)

$packageIdLower = $PackageId.ToLowerInvariant()
$url = "https://api.nuget.org/v3/flatcontainer/$packageIdLower/index.json"
Copy link
Member

Choose a reason for hiding this comment

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

We might want to make the feed a parameter - in case we version before pushing to NuGet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion.


foreach ($assemblyVersion in $propertyGroup.Elements("AssemblyVersion"))
{
$assemblyVersion.Value = Get-NewVersion $assemblyVersion.Value -negativePosition -2
Copy link
Member

Choose a reason for hiding this comment

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

How are you handling the cases where we don't want to change this version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not being handled yet. I know azure does allow passing it as a parameter. I thought at the moment we didn't need it, as we would want the tool to update all the numbers in a consistent way. I think we could easily add such functionality in the future if we need it.

if ($null -ne $conditionedVersionPrefix -and $conditionedVersionPrefix.GetType() -eq [System.Xml.Linq.XElement])
{
if ($latestNuGetVersion -eq $conditionedVersionPrefix.Value)
{
Copy link
Member

Choose a reason for hiding this comment

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

For all of the below - do we want to fail if any are not found, or do any comparisons against latestNuGetVersion?


foreach ($file in $msBuildVersionFilePaths)
{
$xdoc = [System.Xml.Linq.XDocument]::Load($file, [System.Xml.Linq.LoadOptions]::PreserveWhitespace)
Copy link
Member

Choose a reason for hiding this comment

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

If you wanted to try it there is an MSBuild construction API https://learn.microsoft.com/en-us/dotnet/api/microsoft.build.construction. Not sure it adds much value here since we're just modifying existing elements, but it might help with the save issues you were facing.

There's also native support for XML in powershell which might also be an option. I see some folks using it in dotnet repos including to modify projects: https://github.com/search?q=org%3Adotnet+%2F%5C%5Bxml%5C%5D%2F+path%3A*.ps1&type=code


$stringWriter = New-Object System.IO.StringWriter
$xmlWriterSettings = New-Object System.Xml.XmlWriterSettings
$xmlWriterSettings.Indent = $false
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 false is the default.

Suggested change
$xmlWriterSettings.Indent = $false

Comment on lines +92 to +99
If ($output.EndsWith("`r`n"))
{
$output = $output.Substring(0, $output.Length - 2)
}
ElseIf ($output.EndsWith("`n"))
{
$output = $output.Substring(0, $output.Length - 1)
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe [System.Xml.NewLineHandling]::None will fix this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I just realized that it's VS Code that adds the newline whenever I open the file, not the xml API. So I don't need the NewLineHandling nor these if/else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm they're still getting added even if I open the file in notepad, and even if I add that NewLineHandline.None setting. Oh well, I'll have to keep the if/else.

Copy link
Member

@ericstj ericstj Apr 9, 2025

Choose a reason for hiding this comment

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

This worked for me:

[Reflection.Assembly]::LoadWithPartialName("System.Xml.Linq")
$xdoc = [System.Xml.Linq.XDocument]::Load("in.csproj", [System.Xml.Linq.LoadOptions]::PreserveWhitespace)\

$xmlWriterSettings = New-Object System.Xml.XmlWriterSettings
$xmlWriterSettings.OmitXmlDeclaration = $true
$xmlWriterSettings.NewLineHandling = [System.Xml.NewLineHandling]::None

$xmlWriter = [System.Xml.XmlWriter]::Create("out.csproj", $xmlWriterSettings)
$xdoc.WriteTo($xmlWriter)
$xmlWriter.Close()

It was able to round trip the project file produced by .NET new without any changes.

I also found that the built-in XML support in PS worked well as shown in those other scripts.

@carlossanlop carlossanlop closed this by deleting the head repository Jul 6, 2025
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.

2 participants