Implement UpdateDependencies script to automatically take new versions of dependencies by eerhardt · Pull Request #7056 · dotnet/corefx

eerhardt · 2016-03-19T05:15:03Z

There are 2 changes here:

Move dependency validation patterns to Item metadata and add programmatic names.
- This allows automated tools to update the version numbers easier by referencing the names.
  Another advantage is we can write regular expressions easier. Putting regular expressions in the Include attribute can cause a lot of issues.
Initial implementation of the UpdateDependencies.ps1 script. This script will be used to automate taking new versions of dependencies when new builds are available.
- The first (and probably only) dependency to be automated is the CoreFX build numbers in the project.json files.

eerhardt · 2016-03-22T17:56:37Z

@weshaggard @dagood - I have updated the dir.props to follow the pattern we established in dotnet/buildtools#557. Also, I added the new update dependencies script. PTAL.

/cc: @piotrpMSFT @stephentoub

weshaggard · 2016-03-22T18:21:33Z

UpdateDependencies.ps1

+    $nsMgr = new-object Xml.XmlNamespaceManager $xml.NameTable
+    $nsMgr.AddNamespace('b', 'http://schemas.microsoft.com/developer/msbuild/2003')
+
+    $node = $xml.SelectSingleNode('//b:ValidationPattern[@Include = "CoreFxVersions"]/b:ExpectedPrerelease', $nsMgr)


Is this going to cause reformatting of dir.props?

I thought we wanted to go ahead and put this into an independent property so it is a little easier to do a simple find replace. I suspect if we do that we don't even need use an XML API.

The dir.props does get slightly reformatted by this, but there are only 2 changes. You can see what it would look like here: eerhardt@0693e38#diff-0b192804a6349e8c26d2b027afbd89a2L53

There are only two minor changes:

- <Import Project="$(MSBuildThisFileDirectory)Packaging.props"/> + <Import Project="$(MSBuildThisFileDirectory)Packaging.props" />

- <DnuRestoreSource>@(DnuSourceList -> '--source %(Identity)', ' ')</DnuRestoreSource> + <DnuRestoreSource>@(DnuSourceList -> '--source %(Identity)', ' ')</DnuRestoreSource>

My feeling was to not add a level of indirection (i.e. pull the value out into its own property) just for this script. Instead, write the dir.props as it makes sense, and then make the automation conform to it.

I can totally change this to to pull it out into its own variable, if you are strongly against this approach.

I don't think I understood the comment on the other PR when I read it earlier, sorry--now I see that another reason to have it in its own property is that other parts of the build could use it later (without special code in the target). So it's not just for the update script's sake, it's for the build whenever we need this version number.

Yes I would prefer we pull it out into its own property.

eerhardt · 2016-03-23T05:03:06Z

@weshaggard and @dagood - I've responded to feedback and pushed the new changes. The dir.props replacement is a simple Regex now, so no formatting of the dir.props will occur. The new version number is written to the commit and PR messages. PTAL.

weshaggard · 2016-03-23T05:30:34Z

UpdateDependencies.ps1

+        return $false
+    }
+
+    $DirPropsPath = "$PSScriptRoot\dir.props"


I don't have a particular issue with what you have here but I thought I would at least share how to do the same thing in powershell cmdlets as well (I wrote this a while ago as when I was looking at trying to automate this).

$dirprops = Get-Content dir.props | % { $_ -replace "<CoreFxExpectedPrerelease>(.*)</CoreFxExpectedPrerelease>","<CoreFxExpectedPrerelease>$buildNumber</CoreFxExpectedPrerelease>" } Set-Content dir.props $dirprops

I changed this to use your code.

weshaggard · 2016-03-23T05:37:56Z

Just some minor nits/comments on the powershell script but otherwise LGTM. I will likely want to revisit the environment variable piece as some point.

dagood · 2016-03-23T15:30:43Z

LGTM, and I see that we're already on a buildtools version with the validate changes this needs.

eerhardt · 2016-03-23T17:53:45Z

UpdateDependencies.ps1

+
+    $env:GIT_COMMITTER_NAME = $UserName
+    $env:GIT_COMMITTER_EMAIL = $Email
+    git commit -m "$CommitMessage" --author "$UserName <$Email>" | Out-Host


We should check to see if anything was commited, and opt out if there were no changes.

If you add --porcelain I think this is as simple as checking to see if there were any lines of output. (But maybe this comment is just to mark it as future work which is fine too.)

Nice! I used git status --porcelain. Works like a charm.

eerhardt · 2016-03-28T01:52:09Z

I've responded to feedback. PTAL

dagood · 2016-03-28T14:37:14Z

Still LGTM. 👍

weshaggard · 2016-03-28T16:47:07Z

UpdateDependencies.ps1

+    [Parameter(Mandatory=$true)][string]$GitHubUser,
+    [Parameter(Mandatory=$true)][string]$GitHubEmail,
+    [Parameter(Mandatory=$true)][string]$GitHubPassword,
+    [string]$CoreFxVersionUrl='https://raw.githubusercontent.com/dotnet/versions/master/dotnet/corefx/master/Latest.txt', 


I assume this still needs to be actually defined.

Yes. This probably won't be the final location, given our conversation last week. But the idea is that this will be an optional parameter going forward. It felt wrong hard-coding 'https://github.com/eerhardt/versions' in the corefx repo, so I optimistically picked a URL.

Do you think it would be better to just make this mandatory now, since it doesn't work if you don't supply the actual value? Or should I hard-code the path to 'eerhardt/versions' for now? Or leave it as-is?

Yes. I think this should be mandatory at least until we have a real value to default it to.

weshaggard · 2016-03-28T18:26:34Z

Fix the merges issues and update build values and then it looks good.

…atic names. This allows automated tools to update the version numbers easier by referencing the names. Another advantage is we can write regular expressions easier. Putting regular expressions in the Include attribute can cause a lot of issues.

eerhardt · 2016-03-28T20:18:16Z

I've rebased, squashed and pushed the latest.

I don't know why there aren't any CI jobs kicking off for this PR. If it was just the .ps1 script, I would be OK with merging it without CI. But since I'm updating the dir.props file, this should get a build to make sure it doesn't break the version validation script...

weshaggard · 2016-03-28T20:35:41Z

Yes lets please verify CI works before merging. It looks like some jobs have triggered.
\

eerhardt · 2016-03-28T23:12:50Z

@dotnet-bot test this please

eerhardt · 2016-03-29T04:18:26Z

@dotnet-bot test Innerloop OSX Debug Build and Test
@dotnet-bot test Innerloop Windows_NT Release Build and Test

eerhardt · 2016-03-29T06:42:55Z

@dotnet-bot test Innerloop Windows_NT Release Build and Test

eerhardt · 2016-03-29T07:08:36Z

@dotnet-bot test Innerloop Windows_NT Release Build and Test

Implement UpdateDependencies script to automatically take new versions of dependencies Commit migrated from dotnet/corefx@36e9d05

dnfclas added the cla-already-signed label Mar 19, 2016

dotnet-bot added the 2 - In Progress label Mar 19, 2016

eerhardt mentioned this pull request Mar 19, 2016

Add support for ValidationPatternsFile. dotnet/buildtools#557

Merged

eerhardt force-pushed the DependencyVersions branch from b8c6825 to e6c444f Compare March 22, 2016 17:50

eerhardt changed the title ~~Move dependency validation patterns to a separate file.~~ Implement UpdateDependencies script to automatically take new versions of dependencies Mar 22, 2016

weshaggard reviewed Mar 22, 2016
View reviewed changes

eerhardt force-pushed the DependencyVersions branch from e6c444f to 03a7c48 Compare March 23, 2016 05:01

weshaggard reviewed Mar 23, 2016
View reviewed changes

weshaggard mentioned this pull request Mar 23, 2016

Update packages to rc3-23923 #7178

Merged

eerhardt reviewed Mar 23, 2016
View reviewed changes

eerhardt force-pushed the DependencyVersions branch from 15033d6 to 20fc2fd Compare March 28, 2016 01:59

weshaggard reviewed Mar 28, 2016
View reviewed changes

eerhardt added 2 commits March 28, 2016 14:48

Initial UpdateDependencies.ps1 implementation.

ab63a56

eerhardt force-pushed the DependencyVersions branch from 20fc2fd to ab63a56 Compare March 28, 2016 20:12

mmitche closed this Mar 28, 2016

mmitche reopened this Mar 28, 2016

mmitche removed the 2 - In Progress label Mar 28, 2016

dnfclas added the cla-already-signed label Mar 28, 2016

mmitche added the 2 - In Progress label Mar 28, 2016

eerhardt merged commit 36e9d05 into dotnet:master Mar 29, 2016

dotnet-bot removed the 2 - In Progress label Mar 29, 2016

karelz modified the milestone: 1.0.0-rtm Dec 3, 2016

Conversation

eerhardt commented Mar 19, 2016

Uh oh!

eerhardt commented Mar 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt commented Mar 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weshaggard commented Mar 23, 2016

Uh oh!

dagood commented Mar 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt commented Mar 28, 2016

Uh oh!

dagood commented Mar 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weshaggard commented Mar 28, 2016

Uh oh!

eerhardt commented Mar 28, 2016

Uh oh!

weshaggard commented Mar 28, 2016

Uh oh!

eerhardt commented Mar 28, 2016

Uh oh!

eerhardt commented Mar 29, 2016

Uh oh!

eerhardt commented Mar 29, 2016

Uh oh!

eerhardt commented Mar 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants