-
Notifications
You must be signed in to change notification settings - Fork 117
psake build script review #145
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
Comments
OK, I've fleshed out more extension points and renamed Pre/PostCopy. The only one missing that I can think of is Pre/PostTest. Anybody think we need that?
|
Nice! I like it so far. Some feedback:
|
Nice,
|
Good point on PSSA. You do get the benefit of real-time PSSA in VSCode but yeah, being a configurable/optional part of the build pipeline is a good idea. That way, you could run the build from the command line and (optionally) have PSSA fail the build if there are rule errors. |
This separates out the user customizations into a separate file build.settings.ps1. Also made changes related to the discussion in issue #145. Fixed some bugs with the build script as well after a fair bit of testing. Too bad my code-signing cert expired last month. Hopefully DigiCert will come through with a new cert so I can test the Sign task.
* Refactored psake build script. This separates out the user customizations into a separate file build.settings.ps1. Also made changes related to the discussion in issue #145. Fixed some bugs with the build script as well after a fair bit of testing. Too bad my code-signing cert expired last month. Hopefully DigiCert will come through with a new cert so I can test the Sign task. * Eliminate 28 script analyzer warnings.
Information only: After @davegreen has added an Analyze step (using PSScriptAnalyzer), here is the current task list:
The default build flow is now:
|
Input required: Right now the newly added Perhaps the default Also, I think I'd like to allow for script analysis customization via the NewModule template by adding this file to the # The PowerShell Script Analyzer will generate a warning
# diagnostic record for this file due to a bug -
# https://github.com/PowerShell/PSScriptAnalyzer/issues/472
@{
# Only diagnostic records of the specified severity will be generated.
# Uncomment the following line if you only want Errors and Warnings but
# not Information diagnostic records.
#Severity = @('Error','Warning')
# Analyze **only** the following rules. Use IncludeRules when you want
# to invoke only a small subset of the defualt rules.
IncludeRules = @('PSAvoidDefaultValueSwitchParameter',
'PSMissingModuleManifestField',
'PSReservedCmdletChar',
'PSReservedParams',
'PSShouldProcess',
'PSUseApprovedVerbs',
'PSUseDeclaredVarsMoreThanAssigments')
# Do not analyze the following rules. Use ExcludeRules when you have
# commented out the IncludeRules settings above and want to include all
# the default rules except for those you exclude below.
# Note: if a rule is in both IncludeRules and ExcludeRules, the rule
# will be excluded.
#ExcludeRules = @('PSAvoidUsingWriteHost')
} This would give the user an obvious way to configure PSSA. Then we would add a
I also like the settings.json idea because I would probably drop this into the workspace settings file as well: //-------- Files configuration --------
// When enabled, will trim trailing whitespace when you save a file.
"files.trimTrailingWhitespace": true, This should be a mandatory setting for PowerShell script authoring IMO given the issues with whitespace trailing a line continuation char. |
Also, we need to update the NewModule plasterManifest.xml file to indicate if
I think the module probably should not require PSSA and the build script should tolerate not having the module. Simple emit a message about "Skipping Analyze task" if PSSA is not installed (or ScriptAnalyisAction is set None). Although, if it's the case of PSSA not installed AND they have ScriptAnalysisAction set to something other than None, then we should let the user know that the Analyze task is being skipped because they haven't installed the PSSA module. In the NewModule manifest then we would probably want to add a |
I didn't think about users not having PSSA at all, I'm not sure why. We can always detect on the module existence and skip based on that. I set the initial default to Error, mostly because I feel like you have to do some potentially quite bad things to get an error severity out of it. There is currently a 'None' setting which essentially gets you a 'report only' output with no fail, so that would probably be the way forward if the module is present. The I definitely agree on the |
Good point. I could go either way on "enabled by default or not". Let's think about that a bit. I'm in the process of adding in the ScriptAnalysisSettings.psd1 / settings.json. Let me submit that PR in a few minutes - have you look it over. After we merge that in, perhaps you can fix the Analyze task to handle PSSA not being installed. Also, if you wouldn't mind, update plasterManifest.xml to put in a RE None vs Skip - any chance we can rename that. I know coming from the VS space, I would consider "None" to be what you currently mean by "Skip". Perhaps the current None behavior could be renamed to "ReportOnly" (can't think of a better name ATM)? Thanks man! |
Input required: Also, the current "editor-based" PSSA ruleset is different (more limited) than the current build script that ships with the NewModule template. It uses the default ruleset with just one rule disabled: All that said, is this going to be confusing for users of the template within VSCode? |
RE None vs Skip, That would make more sense with the altered variable names, with None being ReportOnly and Skip becoming None. Sure, I can do the Analyze task skip if the module isn't installed and the manifest requiremodule directive. I'm unsure about how to deal with the differences in PSSA between editor based and running the module. I imagine the goal is to eventually get to parity? |
Sounds good. I've merged by PR so you have the floor. :-) WRT to the ruleset difference, hopefully @daviwil will weigh in on that. |
After using the latest build script I'm beginning to wonder about a couple of things. First, script analysis is slow. Should we put this in a later build step? Second, should we be doing anything with New-FileCatalog on system that have this cmdlet installed? We could be creating a .cat file for a module and if script signing is enabled, Authenticode sign the .cat file. Also, I find this name Similarly for Finally, for consistency I'd like to change the I'll take a whack at the build script bits and submit a PR today so we have some code to look at. |
I had other work to do so I went ahead and merged this PR. If we decide we want the "fail build on lack of code coverage", let me know. If we do then I would add a new variable called something like Yeah I know, long variable name but I figure folks won't need to go into the settings very often. So when they do, we want variable names to be pretty obvious. |
OK, that sounds good. I don't use code coverage in the things I write, so I thought if I included the option, it could be used or not, so that's fine. That's probably a better way to control it as well if we were to re-add it. The script analyzer bits sound great too. the variable format |
I didn't know VS code used |
I dithered on the ScriptAnalyzer* names. I changed all three variables to ScriptAnalyzer* and looked at it and didn't like it. Mainly for the variable that controls build failure severity level as that isn't a ScriptAnalyzer thing, it is purely a construct of our build script. OTOH the settings files is definitely ScriptAnalyzer specific file, so I did change that variable to ScriptAnalyzerSettingsPath. And ScriptAnalyzerEnabled seemed less idea than ScriptAnalysisEnabled. That said, I could be convinced to go either way as I'm kind of on the fence as it is on this. Naming is hard. :-) |
Well the current implementation for the debugger host returns "Visual Studio Code Host" but users would rarely execute the build script under the debugger. Most of the time, from VSCode, you run build & test tasks via the Task Runner and it is configured to launch PowerShell.exe. So you get ConsoleHost. |
Should we try to tackle versioning at all? Or just leave it up to the user to change the version number in their .psd1 file (and anywhere else it might need updating)? Should we try to tackle combining individual .ps1 script files into the module's .psm1 file (perhaps as an option)? Does having separate scripts to dot source from within a .psm1 really impact load time that much? |
@rkeithhill There is a module called BuildHelpers that is used as a Helper Module for PSDeploy. It has functions to update the psd1 metadata, update functions to export and get/update the next module version etc. I have been using it in my build scripts to update functions to export(my exported functions are in a public folder), Formats to export (in a formats folder), Module version, and things like that. It has really made that very easy with only a few lines of code. If the option to combine all functions into the psm1 were added, you could still update the functions to export prior to the merge. However, this still doesn't address some issues I have seen discussed in the community, which is how to adapt these types of processes to a CI/CD sort of platform like Appveyor or Teamcity. Since these platforms are typically triggered at commit, how do you handle all of the steps that are generating markdown, building help, making updates to the psd1, signing, publishing etc that are now out of sync with git and your local build. This is an issue that I hear people talk about very often. I have had multiple people message me asking about how to handle this problem, and I don't know the answer and have not be able to find anyone that had a clear solution. The template still doesn't answer that question for people, and I wonder how you would adapt the psake build script to work in that scenario. |
@gerane
Maybe the issues arise from the fact that some of those steps should not be in the build process. For instance, updating the functions to export is a step taken by the developer. It is a conscious decision when adding the new function. Which means that it should be part of the source code, not the build process. If the building tools like PSake offer a way to automate repetitive tasks, it does not mean they should all end in the automated build. To me, changing the version number is one of those. You can automate the various steps, tag, change version number, generate release notes. But obviously, you would not like to release each time a build is launched. If you use a git workflow you could rig the build so that a release happens automatically when you commit to a particular branch. However, I cannot see how the build process could automatically increment the version number. This is a decision the release manager has to make: do i increment patch, minor or major ? Maybe I did not get the question correctly, but it reminded me a lot of a colleague of mine I was helping releasing his module so I thought I would add my 2c. |
I agree that a "publish" step shouldn't be part of a "normal" build.
That's the key. You can have many build steps that are not part of what you would run as an "automated" Travis/AppVeyor build. But they are there when you need to run them manually or via a Task Runner like the one in VSCode. As for updating version numbers, it is actually quite common to update version numbers automatically during a build. It is just that the major & minor version (and sometimes the patch version) are specified in a config file or build variable somewhere so you still have that control. The rest of the version number (typically build metadata) gets auto-generated based on that static info. Checkout GitVersion. |
Yes your are correct, I was referring to version number changes that occur in case of a release, build numbers should be changed by the build. I did not know GitVersion, I will look into it.Thanks for mentioning it. About :
For small modules it may not matter much, but as soon as things get a bit bigger you need a way to split things up. I contribute to the Netscaler module https://github.com/devblackops/NetScaler where we have more than a hundred functions. It would be completely unmanageable if they were not organised by file. I did not consider the performance impact. That could be an issue but did not see a really noticeable impact for the 135 files we currently have. In the Netscaler module we use the following loading pattern, because Public and Private functions are separated into distinct directories:
In Forge and in all modules with more than 1 public function that I did during the last months I use the following pattern:
In any case, even if the template generates the loading code, it is very easy for the module developer to just remove it if he does not like it. From my point of view that way of doing things has two other advantages:
What do you think ? |
I run my pester tests on the release folder since that is where the full module resides. That means code, help, formats, and manifest. Since I create pester tests to make sure the help is complete ( is it auto generated vs actually populated, do parameters have help, etc) I need the generated MAML file. Shouldn't Build include generating the MAML file from the docs? |
Notes on the build : The build tasks are implicitly linked to the structure of the project, that is not specified. The order of execution of the tasks is not specified. See Specify that the template is oriented mono-module. The Apikey should be linked to the repository not to the template, if I change the key I have to change all projects using the same repository. Tasks to improve CoreStagefiles: Move : New-Item $ModuleOutDir -ItemType Directory -Verbose:$VerbosePreference > $null to the 'Clean' task : Get-ChildItem $OutDir | Remove-Item -Recurse -Force -Verbose:($VerbosePreference -eq 'Continue')
#NewDirectory is a function to create directory
NewDirectory -Path $ModuleOutDir -TaskName $psake.context.currentTaskName CorePublish : $publishParams = @{
Path = $ModuleOutDir
NuGetApiKey = $NuGetApiKey
} When I want publish inside a CI (Appveyor) or execute the build inside a Job, the function 'PromptUserForCredentialAndStorePassword' is not appropriate. Analyze : Settings $ProjectName= 'PROJECTNAME'
$ProjectUrl= 'https://github.com/USER/Project.git' Name of the Modules for additionnal custom rule : [String[]]$PSSACustomRules=@(
#GetModulePath return the full path of the module
GetModulePath -Name OptimizationRules
GetModulePath -Name ParameterSetRules
) ApiKey setting file name : # Path to encrypted APIKey file(s).
$NuGetApiKeyPath = "$env:LOCALAPPDATA\Plaster\SecuredBuildSettings\$PublishRepository-ApiKey.clixml" Template can targeted many projects $SettingsPath = "$env:LOCALAPPDATA\Plaster\NewModuleTemplate\SecuredBuildSettings.clixml" to $SettingsPath = "$env:LOCALAPPDATA\Plaster\SecuredBuildSettings\$ProjectName.clixml" Add BuildConfiguration : 'Release' or 'Debug' $BuildConfiguration='Release' I can deploy a module with trace (DEBUG) or without trace (RELEASE) bug : -Verbose:$Verboseprefrence to -Verbose:($VerbosePreference -eq 'Continue') During the execution of the 'Publish" task, I get this Warning : WARNING: ReleaseNotes is now supported in the module manifest file (.psd1). Update the module manifest file of module
'Template' in 'G:\PS\Template\Release\Template' with the newest ReleaseNotes changes. You can run Update-ModuleManifest
-ReleaseNotes to update the manifest with ReleaseNotes. Any idea (ps v5.1) ? |
The note on the export of the apikey in a separate file concerns the case where I want to use two repository, one for testing (MyGet) and one for the public (PSGallery). |
For information, inside a PS Job the $Profile variable do not exist, this statements fail : $InstallPath = Join-Path (Split-Path $profile.CurrentUserAllHosts -Parent) `
"Modules\$ModuleName\$((Test-ModuleManifest -Path $SrcRootDir\$ModuleName.psd1).Version.ToString())" I get this exception : ~~~ [<<==>>] Exception: Cannot bind argument to parameter 'Path' because it is null. Test : test-path variable:Profile
True
start-job {test-path variable:Profile}|Receive-Job -Wait -AutoRemoveJob
False Same issue under Appveyor, possible workaround : install:
- ps: |
PowerShell.exe -Command {
Export-Clixml -InputObject $Profile -Path $env:temp\PSProfile.xml }
build_script:
- ps: |
$Profile=Import-Clixml $env:temp\PSProfile.xml
.\build.ps1 .... |
TL;DR - skim through and look at the bolded questions/issues. FYI, I'm in the midst of making changes to build.psake.ps1 so please hold off on making changes to this file.
Currently our build script has the following tasks:
The
default
task isBuild
. The primary build flow is:The two simple user extension points are:
The BuildHelp flow is:
BTW since we usually refer to docs as "help" in PowerShell, shouldn't these be called GenerateHelp or better yet,
GenerateMarkdown
andBuildHelp
? GenerateFoo and BuildFoo sound a bit like they would do the same thing, at least to me. Update: I've temporarily made this change.The install task flow is:
BTW, I think we need an option (property) on the Install task to allow it to "clean" the destination before copying over the module. The default should probably be to not clean the destination first.
The test task flow is:
And the publish flow is:
This provides two more empty, task extension points for the user:
Pre/PostPublish
.Hmm, forgot that I had used this "impl" pattern before. I like that and may convert PreCopySource/PostCopySource (never liked those names) to more of PreBuild/PostBuild where these would fit in like so:
In fact, I might do the same for Install (Pre/PostInstall). These are simple to add and give the user easy ways to hook into the build flow without having to drop down and modify the common code. Update: I've temporarily made this change.
OK this has me thinking (given something @daviwil mentioned a few days ago), we should look into splitting up the build.psake.ps1 file and putting the things the user is going to change in one file and the fixed stuff in another file (perhaps module-build.psake.ps1).
Finally, in testing the build script I found some cases where a newly created module wouldn't build without failures. When we are creating build steps like the help build for instance, we need to be sure we tolerate conditions like no exported commands.
The text was updated successfully, but these errors were encountered: