Skip to content

add build.ps1 to align with PSES and other PowerShell projects #1199

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

Merged
merged 3 commits into from
Mar 1, 2018

Conversation

TylerLeonhardt
Copy link
Member

Resolves: #1127

Here's the build.ps1 PR for PowerShellEditorServices:
PowerShell/PowerShellEditorServices#623

In scope:

  • running build.ps1 checks for missing tools and runs Invoke-Build Build
  • running build.ps1 -Clean checks for missing tools and runs Invoke-Build Clean and then Invoke-Build Build
  • running build.ps1 -Test checks for missing tools and runs Invoke-Build Build and then Invoke-Build Test
  • running build.ps1 -Bootstrap informs you of what tools you are missing
    • the dependencies it checks for are:
      • VSCode
      • Node.js 6 or greater
      • PowerShellGet
      • InvokeBuild

if($Test) {
Invoke-Build Test
}
}
Copy link
Member

Choose a reason for hiding this comment

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

no new line

Copy link
Member Author

Choose a reason for hiding this comment

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

added

@@ -0,0 +1,121 @@
#!/usr/bin/env pwsh
Copy link
Member

Choose a reason for hiding this comment

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

No copyright header

Copy link
Member Author

Choose a reason for hiding this comment

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

added. Wasn't sure which should be on top. Let me know if I have it wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

My testing indicates that the shebang has to be on the first line of the script.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, shebang must be first line. This would be the exception to the copyright header rule being first.

@@ -0,0 +1,121 @@
#!/usr/bin/env pwsh
Copy link
Member

Choose a reason for hiding this comment

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

Yes, shebang must be first line. This would be the exception to the copyright header rule being first.

build.ps1 Outdated
InvokeBuild = "InvokeBuild latest"
}

if ((-not $PSVersionTable["OS"]) -or $PSVersionTable["OS"].Contains("Windows")) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we just rely on $IsWindows? If it's $null, then we can assume it's Windows since that would be returned by Windows PowerShell. Same for $IsLinux and $IsMacOS.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just realized OS isn't used in this script.

build.ps1 Outdated
param(
[Parameter()]
[switch]
$Bootstrap,
Copy link
Member

Choose a reason for hiding this comment

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

Looking at your logic below on lines 101-124, it seems that -Bootstrap should be it's own ParameterSet as it's mutually exclusive to -Clean and -Test?

Copy link
Member Author

Choose a reason for hiding this comment

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

got it

@rkeithhill
Copy link
Contributor

@tylerl0706 I think something went wrong with a push or something. Shouldn't there only be 1 file changed in this PR?

@TylerLeonhardt
Copy link
Member Author

😮

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@rkeithhill rkeithhill left a comment

Choose a reason for hiding this comment

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

LGTM

@TylerLeonhardt TylerLeonhardt merged commit 6ceb0d6 into PowerShell:master Mar 1, 2018
@TylerLeonhardt TylerLeonhardt deleted the add-buildps1 branch March 1, 2018 17:52
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.

5 participants