PoshCode / ModuleBuilder

A PowerShell Module to help scripters write, version, sign, package, and publish.
MIT License
445 stars 54 forks source link

Make build.psd1 optional and fix bugs in 1.5.2 #89

Closed Jaykul closed 4 years ago

Jaykul commented 4 years ago

Fixes #87 by not using ModuleManifest explicitly anymore Fixes #88 by not loosing the value of the prefix Address #44 because #87 was so darn close. The build.psd1 is optional

I don't think this will break anything, but I'd like some reassurance. If there's any doubt, I'll do a 2.0.0-alpha01 and we can beg people to test it.

Regardless, even if nobody finds any bugs, I'm willing to bump major because of the build.psd1 change if y'all think it's necessary

Jaykul commented 4 years ago

Wow, that test run was surprising. These work on my box! I'll have to try again tomorrow. Sorry.

bravo-kernel commented 4 years ago

Either one is fine with me and I'm available to test whatever is needed, just let me know.

IMO a major version bump would only be justified if this introduces a breaking change.

If we are going 2.x perhaps we can use that to consider introducing more breaking changes. I don't know if there are on the todo-list here but for example... changing the default upper-cased directories to lower-case (e.g. Source to source, Private to private). Would IMO better accommodate platform-agnostic use besides resulting in properly sorted Github directory views.

image

bravo-kernel commented 4 years ago

ps: the failing build is probably due to the artifact.

1.5.2 produces an artifact without the module name as root folder inside the zip file (instead version number is now root folder inside the zip). This broke my ci as well.

gaelcolas commented 4 years ago

I agree with @bravo-kernel 's statement regarding breaking change. 1.5.2 was already one for us so... We're still pinned to 1.0.0, the issue with publish module's different interpretation of semver is a pain.

But while we're here, I'd like to suggest a different way to detect module manifest (to deduct the module name). I found that finding psd1s that were mostly valid with test-modulemanifest (ignoring specific errors such as rootmodule NotFound & no version set) is a more reliable approach. Guessing from the parent folder's name is not reliable when some tools (azDO) git fetch in a dir that's just the first later than it should be...

bravo-kernel commented 4 years ago

@Jaykul are you confused about the statement or the fact that the artifacts have changed?

Jaykul commented 4 years ago

Yeah, I'm not sure when that changed -- or how!

There's code in the Build-Module step in the Azure pipeline to make sure the name shows up:

# Build-Module currently doesn't guarantee the module name will be in the output
$manifest = Import-LocalizedData -Base $module.Directory.FullName -File $module.Name
$destination = Join-Path "${{ parameters.destination }}" ([IO.Path]::GetFileNameWithoutExtension( $manifest.Path ))

Build-Module -SourcePath $module -Destination $destination -SemVer "${{ parameters.semVer }}" -Verbose

You can see from the build logs that it worked on 1.1.5 and did not work on 1.5.3.

But as far as I can tell Build-Module-step.yml hasn't changed to speak of.

EDIT:

OMG. I just realized why this happened. I changed the build.psd1 to use a different alias for SourcePath (ModuleManifest) instead of Path -- but the pipeline yaml has Path hardcoded.

Jaykul commented 4 years ago

You're definitely right that's why it's failing too. I see now that the failed tests show errors coming from ModuleBuilder 1.5.2, not 1.5.3

bravo-kernel commented 4 years ago

That's great news, once identified... 👍

bravo-kernel commented 4 years ago

I hope I'm not starting to annoy you folks but this was the main reason I once suggested moving all CI templates over to this repository. Cross-repo changes become near-impossible to detect using shared templates and will (al)most certainly break "search and replace"/refactoring.

Perhaps this can be reconsidered. If wanted, I can move them right into here.

Jaykul commented 4 years ago

It would still have broken even if all the build files were copied in here. The only difference is that if they were in here, I would have just changed the hardcoded value in the yaml file just now instead of the psd1 -- meaning I would then encounter the same problem in every other repo one at a time as I touch them.

I'm not convinced 😛

Honestly, if including it as a subrepo would work, I would be happy to do that (it would solve both problems). But the last time I tested, Azure's build system can't handle it -- or at least, you can't use yaml files from the sub-repo.

I guess I could include it as a subrepo and still reference the build files from the remote -- that would let you see the problems. But that might cause confusion because we'd have to manage pinning the commit of the other repo twice...

Jaykul commented 4 years ago

Bah. I forgot. The bug in 1.5.2 prevents using 1.5.2 to build it with Path -- LOL

bravo-kernel commented 4 years ago

Agreed that the error would still have occurred. I'm convinced it would have been found sooner though, using simple repo search. Anyway, it don't really matter much to me, as long as the builds are succeeding I'm happy, thanks for diving into this 💃

Off to work

Jaykul commented 4 years ago

There we go. That's much better. I'm sure I've broken something that's not covered by tests, but at least now you can try it?

bravo-kernel commented 4 years ago

Of course but I wonder how to test your patched version against/using my CI as that will probably just download 1.5.2 from the gallery.

Jaykul commented 4 years ago

I could push it to the gallery as a pre-release, but first I want a little more confirmation. I'm using it at work today, seems ok so far.

bravo-kernel commented 4 years ago

Any news thus far ?

bravo-kernel commented 4 years ago

Friendly ping 💃

Jaykul commented 4 years ago

Ok, fine. I've published it as 1.6.0-beta since nobody has reported either success or failure with the PR build.