psake / PowerShellBuild

Common build tasks for psake and Invoke-Build that build and test PowerShell modules
MIT License
133 stars 24 forks source link

IB Tasks Appear Broken? #16

Open pauby opened 5 years ago

pauby commented 5 years ago

Latest 0.3.0 Beta version. The Build task in PowerShell.IB.Tasks is empty task Build. As a consequence nothing happens when running invoke-build -File .\.build.ps1 -task build

Expected Behavior

In previous version 0.2.0 the Build task was task Build StageFiles, BuildHelp which built the module. I expect the same from the new version.

Current Behavior

You get this:

image

Possible Solution

Change the PowerShell.IB.Tasks file to either use the same build tasks as previous version or use the tasks specified in $PSBPreference.Build.Dependencies which by default is 'StageFiles', 'BuildHelp' (this doesn't appear to be used anywhere but I could be missing it).

JustinGrote commented 5 years ago

I wrote the dynamic IB generator for PSAke tasks so probably missed an edge case with the latest update, I'll take a look!

On Tue, Dec 18, 2018, 3:39 AM Paul Broadwith notifications@github.com wrote:

Latest 0.3.0 Beta version. The Build task in PowerShell.IB.Tasks is empty task Build. As a consequence nothing happens when running invoke-build -File ..build.ps1 -task build Expected Behavior

In previous version 0.2.0 the Build task was task Build StageFiles, BuildHelp which built the module. Current Behavior

You get this:

[image: image] https://user-images.githubusercontent.com/12760779/50151622-53d23080-02b9-11e9-806a-201a15dd83a1.png Possible Solution

Change the PowerShell.IB.Tasks file to either use the same build tasks as previous version or use the tasks specified in $PSBPreference.Build.Dependencies which by default is 'StageFiles', 'BuildHelp' (this doesn't appear to be used anywhere but I could be missing it).

  • Module version used: 0.3.0-beta
  • Operating System and PowerShell version: Windows 10, Windows PowerShell 5.1.17134.407

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/psake/PowerShellBuild/issues/16, or mute the thread https://github.com/notifications/unsubscribe-auth/AOjVUk-LLv2xd6zSJURHkeVZNJ8HVaTqks5u6NQIgaJpZM4ZYIVs .

devblackops commented 5 years ago

Thanks for finding this @pauby and thanks for taking a look @JustinGrote!

I created issue #17 to highlight we need to improve the Pester tests for this project. Right now, there is only basic tests that do not actually verify functionality.

pauby commented 5 years ago

I raised #20 earlier but I have come across another issue that is related to the conversion between psake and InvokeBuild.

The task Publish has an assert statement which is still in psake format (I believe - I don't use psake).

This seems to be a wider issue around conversion.

I'm not sure if it's worhwhile continuing to convert the tasks or whether it would be better simply to keep both sets of tasks updated manually, given that the amount they will change will likely be minimised due to most of the grunt work being done inside functions?

devblackops commented 5 years ago

@JustinGrote Can speak to how much work would be involved in shoring up the conversion script.

nightroman commented 5 years ago

(1) The original script Convert-psake.ps1 is not intended for unsupervised use. On the contrary, it says that commands like Assert and Exec should be verified after conversion. Some of them are fine (omitting param names), some of them are not (the case in question).

(2) True parsing and conversion of psake scripts is probably doable (not 100%) but too tedious. I am not planning this in Convert-psake.ps1.

(3) What can be done for the current issue

In the code supposed to be converted to IB you may omit parameter names in Assert keeping IB compatibility in mind. Then the converted code will be compatible:

Assert ($PSBPreference.Publish.PSRepositoryApiKey -or $PSBPreference.Publish.PSRepositoryCredential) "API key or credential not defined to authenticate with [$($PSBPreference.Publish.PSRepository)] with."

Alternatively, in the custom script derived from the original Convert-psake.ps1 you may add the code adding the following two lines to the result script:

function Assert-PSake([Parameter()]$conditionToCheck, $failureMessage) { Assert-Build $conditionToCheck $failureMessage }
Set-Alias assert Assert-PSake

As a result, the below script ("generated") is a valid IB script using psake Assert syntax:

function Assert-PSake([Parameter()]$conditionToCheck, $failureMessage) { Assert-Build $conditionToCheck $failureMessage }
Set-Alias assert Assert-PSake

task t1 {
    Assert -conditionToCheck $false -failureMessage "Oops..."
}
JustinGrote commented 5 years ago

Sorry for the delay.

So the way the new script works is proxying the psake statement such as Task with "Convert-Task", which then parses the psake statement and spits out the IB equivalent.

The Invoke-Command however is processing variables such as $PSBPreference prior to passing it to Convert-Task, thus resulting in a null because it hasn't been set in the build environment, and hence why build comes up with no dependencies. $PSBPreference wasn't a thing when I wrote the initial script, and it didn't fail the pester test because a Build task actually did get created, so a more specific test to ensure all related parameters are also generated would catch this in the future.

I'll have to figure out a way to take variables specified in PSake command parameters and "escape" them so that the final generated code preserves them rather than evaluate them. It would likely go around line 33 and use AST to find variables and escape them out as single quoted strings or something, then post-process the result of Convert-X and re-insert the variables into the statement before output.

gaelcolas commented 5 years ago

Would you consider manually-written InvokeBuild tasks, instead of generated ones. As far as I understand it, it's a question of having the task runners wrap around the PowerShellBuild public functions? The risk is to have some discrepancy in implementation and behaviour between IB and PSake, but they would probably be more manageable and better quality?

One thing I'm missing in the current IB tasks is that they don't 'inherit' their properties/parameters as I do in my gaelcolas/SampleModule. So overriding parameters seems a bit more tedious.

i.e. For an imported build task like this

Param (
    [string]
    $VariableNamePrefix =  $(try {property VariableNamePrefix} catch {''}),

    [switch]
    $ForceEnvironmentVariables = $(try {property ForceEnvironmentVariables} catch {$false})
)
task Set_Build_Environment_Variables {
    $BH_Params = @{
        variableNamePrefix = $VariableNamePrefix
        ErrorVariable      = 'err'
        ErrorAction        = 'SilentlyContinue'
        Force              = $ForceEnvironmentVariables
        Verbose            = $verbose
    }

    Set-BuildEnvironment @BH_Params
    foreach ($e in $err) {
        Write-Build Red $e
    }
}

I can override the parameters of that task in my IB entry point... (i.e. where I define Task .) With PowerShellBuild we could set the defaults to something like:

Param (
    [string]
    $VariableNamePrefix =  $(try {property VariableNamePrefix} catch {$PSBPreference.BuildHelpers.Prefix}),

    [switch]
    $ForceEnvironmentVariables = $(try {property ForceEnvironmentVariables} catch {$PSBPreference.BuildHelpers.ForceEnvironmentVariables })
)
# [...]

So in your default build.ps1 at the root of your repo, if you want to be able to override the defaults at runtime, you can have it like so:

Param(
    [string]$VariableNamePrefix = 'BH'
)

Task . Set_Build_Environment_Variables,
         Do_something_else,
         finish_the_thing

And invoking an override at build time: build.ps1 -VariableNamePrefix ''

(BuildHelpers prefix is a stupid example, but simple enough to understand the concepts)

gaelcolas commented 5 years ago

I forgot to mention as it was implied, that EVERY tasks using $VariableNamePrefix would now get (inherit) the override, if using the same declaration of parameter for the task file.

$VariableNamePrefix =  $(try {property VariableNamePrefix} catch {$PSBPreference.BuildHelpers.Prefix}),
nightroman commented 5 years ago

@gaelcolas

One thing I'm missing in the current IB tasks is that they don't 'inherit' their properties/parameters as I do in my gaelcolas/SampleModule.

We may talk and think about this at IB issues, please post a question and thoughts.

But it looks like you "work around" this using some overcomplicated constructs. Namely, instead of this (and other similar)

$VariableNamePrefix =  $(try {property VariableNamePrefix} catch {''})

you can just use

$VariableNamePrefix = (property VariableNamePrefix '')

It's the same but simpler and more effective/clean.

gaelcolas commented 5 years ago

The question (mainly for @devblackops) whether he'd accept written IB tasks instead of generated ones still stands.

The complex construct is old code I haven't really reviewed yet. And what I meant by IB tasks was about the generated ones. So it's an issue with the PSake->IB convertor. I prefer avoiding converting tasks for the reasons you mentioned.

devblackops commented 5 years ago

The original implementation manually created the IB tasks until #11. If the automatic converter is not sufficient or can not be relied upon 100% and requires manual touch, then perhaps it is better to manually convert them. I'm OK with this.

I'm also open to improving the Pester tests that validate the conversion. Perhaps we can make that more robust to improve confidence.

pauby commented 5 years ago

I've just come across this issue again so I thought I'd throw my 2p / 5 cents into the mix and feel that we should just go with the manually created tasks. As they are unlikely to change too much I don't think it would be too much maintanence overhead?

JustinGrote commented 5 years ago

If we go manual, probably the best way is to apply Pester tests to both task types to ensure the same output result.

Short of someone helping, this is totally on me to fix and I totally haven't been able to make it a priority on my current duties.

pauby commented 5 years ago

I am happy to create the IB tasks if this is the way we want to go?

devblackops commented 5 years ago

@pauby @JustinGrote I'd say go manual for now. Bonus if we had a simple module to run through the tests with to validate as well.

JustinGrote commented 5 years ago

I'd say manual as well since I haven't had time to dedicate to fixing the wrapper. If I do then it will detect if a task has been manually written and "skip" it, only generating tasks that don't have a IB equivalent.

On Tue, Aug 27, 2019, 9:00 PM Brandon Olin notifications@github.com wrote:

@pauby https://github.com/pauby @JustinGrote https://github.com/JustinGrote I'd say go manual for now. Bonus if we had a simple module to run through the tests with to validate as well.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/psake/PowerShellBuild/issues/16?email_source=notifications&email_token=ADUNKUXEXLAHI2GM5WNEJA3QGXZ7RA5CNFSM4GLAQVWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5J2D5I#issuecomment-525574645, or mute the thread https://github.com/notifications/unsubscribe-auth/ADUNKUXVM365XSPY74OBAOTQGXZ7RANCNFSM4GLAQVWA .

pauby commented 5 years ago

I've submitted #35 for this to use standalone tasks for InvokeBuild for the time being. Note sure if this is a long term fix or a temporay fix until the wrapper is fixed. Either way it gives us something to work with just now.

I am expecting I've missed something to disable the wrapper for the time being but I've changed what I could find.

While the PR fixes this issue technically, going forward maybe it should be left open to discuss?

JustinGrote commented 4 years ago

See the PR. After fixing that or adding this to your invoke-build file header:

if (-not (Get-Module PowershellBuild)) {Import-Module PowershellBuild}
. PowerShellBuild.IB.Tasks
$PSBPreference.Build.CompileModule = $true
task Build $psbpreference.build.dependencies

task . Build

All the tasks as currently defined will work (at least for me)

pauby commented 4 years ago

@JustinGrote Does that change fix https://github.com/psake/PowerShellBuild/blob/master/PowerShellBuild/psakeFile.ps1#L154 when converting?

JustinGrote commented 4 years ago

@devblackops doesn't generate an artifact as part of the appveryor build so I can't check for sure, let me try compiling it locally. Is the problem that the assert doesn't come over?

On Wed, Oct 2, 2019 at 1:16 AM Paul Broadwith notifications@github.com wrote:

@JustinGrote https://github.com/JustinGrote Does that change fix https://github.com/psake/PowerShellBuild/blob/master/PowerShellBuild/psakeFile.ps1#L154 when converting?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/psake/PowerShellBuild/issues/16?email_source=notifications&email_token=ADUNKUUNVWMLGRDSSZK2O53QMRKEFA5CNFSM4GLAQVWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAD6EPQ#issuecomment-537387582, or mute the thread https://github.com/notifications/unsubscribe-auth/ADUNKUSPAYBSBCS74AHXHR3QMRKEFANCNFSM4GLAQVWA .

pauby commented 4 years ago

@JustinGrote No, it's that the Assert in InvokeBuild is different. See the PR I put in for the correct syntax.

JustinGrote commented 4 years ago

Ah I see. Since it's not part of the original psake task, it wouldn't incorporate your change, because the file gets dynamically generated at build time. It would need to be part of the psake task, which it "kind of" is already and I confirmed this carries through to the IB task.

if ($settings.PSGalleryApiKey) {
    Publish-Module -Path $settings.ModuleOutDir -NuGetApiKey

$settings.PSGalleryApiKey -Repository PSGallery } else { throw 'Did not find PSGallery API key!' }

On Wed, Oct 2, 2019 at 7:02 AM Paul Broadwith notifications@github.com wrote:

@JustinGrote https://github.com/JustinGrote No, it's that the Assert in InvokeBuild is different. See the PR I put in https://github.com/psake/PowerShellBuild/pull/35/files#diff-a66a6537ef38b034878669b1ae569beeR156

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/psake/PowerShellBuild/issues/16?email_source=notifications&email_token=ADUNKUT5XUXIHERJ2YGGMRLQMSSWDA5CNFSM4GLAQVWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAE3HJQ#issuecomment-537506726, or mute the thread https://github.com/notifications/unsubscribe-auth/ADUNKUWZXL2R7DXDWPOGHE3QMSSWDANCNFSM4GLAQVWA .

JustinGrote commented 4 years ago

Also if you're interested in Native IB tasks, check out my PowerCD module, though I'm working to abstract them into functions so they can be included here because I believe in @devblackops goal of having a consistent standard library of tasks to work with, and then opinionated templates from there.

https://github.com/justingrote/powercd

On Wed, Oct 2, 2019 at 7:15 AM Justin Grote justin@grote.name wrote:

Ah I see. Since it's not part of the original psake task, it wouldn't incorporate your change, because the file gets dynamically generated at build time. It would need to be part of the psake task, which it "kind of" is already and I confirmed this carries through to the IB task.

if ($settings.PSGalleryApiKey) {
    Publish-Module -Path $settings.ModuleOutDir -NuGetApiKey

$settings.PSGalleryApiKey -Repository PSGallery } else { throw 'Did not find PSGallery API key!' }

On Wed, Oct 2, 2019 at 7:02 AM Paul Broadwith notifications@github.com wrote:

@JustinGrote https://github.com/JustinGrote No, it's that the Assert in InvokeBuild is different. See the PR I put in https://github.com/psake/PowerShellBuild/pull/35/files#diff-a66a6537ef38b034878669b1ae569beeR156

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/psake/PowerShellBuild/issues/16?email_source=notifications&email_token=ADUNKUT5XUXIHERJ2YGGMRLQMSSWDA5CNFSM4GLAQVWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAE3HJQ#issuecomment-537506726, or mute the thread https://github.com/notifications/unsubscribe-auth/ADUNKUWZXL2R7DXDWPOGHE3QMSSWDANCNFSM4GLAQVWA .

pauby commented 4 years ago

@JustinGrote @devblackops Can I clarify the current state on this? My understanding was that separate standalone psake / IB tasks were going to be used (and I submitted #35 for that) but I'm getting the impression that has changed? 😕

(Note I'm not asking 'why has my PR not been merged'!)

JustinGrote commented 4 years ago

It can always be both, we can modify the script to do automatic ones as a separate file and then have the manual ones as another one, that way they can proceed in parallel.

On Sun, Oct 6, 2019, 7:58 AM Paul Broadwith notifications@github.com wrote:

@JustinGrote https://github.com/JustinGrote @devblackops https://github.com/devblackops Can I clarify the current state on this? My understanding was that separate standalone psake / IB tasks were going to be used (and I submitted #35 https://github.com/psake/PowerShellBuild/pull/35 for that) but I'm getting the impression that has changed? 😕

(Note I'm not asking 'why has my PR not been merged'!)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/psake/PowerShellBuild/issues/16?email_source=notifications&email_token=ADUNKUTMASJ3J5MOGSZXNKLQNH4IRA5CNFSM4GLAQVWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAOMA5I#issuecomment-538755189, or mute the thread https://github.com/notifications/unsubscribe-auth/ADUNKURVS3VLKBL36XMNPNDQNH4IRANCNFSM4GLAQVWA .

devblackops commented 4 years ago

@pauby Plan is still to proceed with the manual ones still. I've just pushed a test module to /tests/TestModule which we can use to run both psake/IB tasks against and validate the behavior with Pester.

(Note I'm not asking 'why has my PR not been merged'!)

Haha. I've been a lazy bum and will merge this now.

pauby commented 4 years ago

Is there more to do on this?