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.

Sign up for GitHub

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

add clear output flag to build.ps1 #15255

Merged
merged 3 commits into from
Aug 20, 2021
Merged

add clear output flag to build.ps1 #15255

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3e9cc98
add clear output flag
ArcturusZhang Aug 10, 2021
7b51f19
remove the path parameter and make sure the process-sdk function only…
ArcturusZhang Aug 11, 2021
dd54298
change flag name
ArcturusZhang Aug 19, 2021
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
30 changes: 18 additions & 12 deletions eng/scripts/build.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,25 @@
#Requires -Version 7.0
param([string]$filter, [switch]$clean, [switch]$vet, [switch]$generate, [switch]$skipBuild, [switch]$format, [switch]$tidy, [string]$config = "autorest.md", [string]$outputFolder)
param([string]$filter, [switch]$clean, [switch]$vet, [switch]$generate, [switch]$skipBuild, [switch]$clearOutput, [switch]$format, [switch]$tidy, [string]$config = "autorest.md", [string]$outputFolder)
Copy link
Member

Choose a reason for hiding this comment

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

Given this script is used in other contexts, a more descriptive parameter name like cleanAutogeneratedFiles or something might be better.


. $PSScriptRoot/meta_generation.ps1
. $PSScriptRoot/get_module_dirs.ps1


function Process-Sdk ($path) {
function Process-Sdk () {
$currentDirectory = Get-Location
if ($clean) {
Write-Host "##[command]Executing go clean -v ./... in " $path
Write-Host "##[command]Executing go clean -v ./... in " $currentDirectory
go clean -v ./...
}

if ($clearOutput) {
Write-Host "##[command]Cleaning auto-generated files in" $currentDirectory
rm "zz_generated_*"
Copy link
Member

Choose a reason for hiding this comment

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

Only some of these commands actually seem to use $path, others (like this and go clean just use the current directory. I assume this is a bug?

Copy link
Member Author

@ArcturusZhang ArcturusZhang Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I actually do not know what the command go clean does here. I would defer @chamons to answer this and discuss whether we need that option.

Based on the only invocation of this function, $path is identical with local directory, therefore we do not have a bug here.

I agree we should unify the usage of path here. But actually I do not like the way that we are passing the absolute path everywhere - how about we just remove the $path parameter from this function and make sure this function only takes effects in whatever the current directory is. The caller should ensure we are in the correct directory first, and then invoke this function, like this script does.

Copy link
Member

Choose a reason for hiding this comment

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

I'm realizing $path is probably more for being able to parse pipeline logs and understand which context things failed in, though that could probably also be achieved by logging directory changes when they happen.

I think the only consideration with absolute pathing is when you're running scripts to work against multiple project directories, error handling/control flow becomes a little simpler because you won't get dropped into subdirectories accidentally and you don't have to handle pushd/popd all over the place. Additionally if the scripts ever need to reference calls to other scripts/tools, then having one consistent script directory is more convenient (though I think just using $PSScriptRoot may solve that problem).

Copy link
Member Author

Choose a reason for hiding this comment

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

The case here is that based on my understanding of how the go tools work (which might not be correct), some of them should either accept a relative directory (go vet for instance), or a full package identifier which is not necessary to be an absolute path. Therefore I prefer to use the relative path in the invocations of commands.
I added another variable to first get the absolute path of current directory once we go into the function so that we would have that in our verbose logging.

}

if ($generate) {
Write-Host "##[command]Executing autorest.go in " $path
$autorestPath = $path + "/" + $config
Write-Host "##[command]Executing autorest.go in " $currentDirectory
$autorestPath = "./" + $config

if (ShouldGenerate-AutorestConfig $autorestPath) {
Generate-AutorestConfig $autorestPath
Expand All @@ -22,7 +28,7 @@ function Process-Sdk ($path) {

$autorestVersion = "@autorest/go@4.0.0-preview.25"
if ($outputFolder -eq '') {
$outputFolder = $path
$outputFolder = $currentDirectory
}
autorest --use=$autorestVersion --go --track2 --go-sdk-folder=$root --output-folder=$outputFolder --file-prefix="zz_generated_" --clear-output-folder=false $autorestPath
if ($LASTEXITCODE) {
Expand All @@ -35,24 +41,24 @@ function Process-Sdk ($path) {
}

if ($format) {
Write-Host "##[command]Executing gofmt -s -w . in " $path
gofmt -s -w $path
Write-Host "##[command]Executing gofmt -s -w . in " $currentDirectory
gofmt -s -w .
}

if ($tidy) {
Write-Host "##[command]Executing go mod tidy in " $path
Write-Host "##[command]Executing go mod tidy in " $currentDirectory
go mod tidy
}

if (!$skipBuild) {
Write-Host "##[command]Executing go build -x -v ./... in " $path
Write-Host "##[command]Executing go build -x -v ./... in " $currentDirectory
go build -x -v ./...
Write-Host "##[command]Build Complete!"

}

if ($vet) {
Write-Host "##[command]Executing go vet ./... in " $path
Write-Host "##[command]Executing go vet ./... in " $currentDirectory
go vet ./...
}

Expand All @@ -79,7 +85,7 @@ if (![string]::IsNullOrWhiteSpace($filter)) {
try {
$keys | ForEach-Object { $sdks[$_] } | ForEach-Object {
Push-Location $_.path
Process-Sdk $_.path
Process-Sdk
Pop-Location
}
}
Expand Down