Build-Module can produce invalid module without raising error

[daviesj]

In at least two cases, Build-Module produces a module which cannot be imported and does not generate an error:

• When you have a valid Build.psd1 and manifest but no source .ps1 files, Build-Module creates the output folder and the output manifest, but no .psm1.
• When you have certain syntax errors (such as mismatched braces or misspelled function), Build-Module produces a .psm1 that will fail to import and does not complain about it.

It would be helpful if the tool responsible for building a script module could avoid producing output which cannot be imported and report an error when it is unable to do so.

If I were designing a build tool like this:

• In the case of no source files, it would produce an error message and no output would be created. Another option would be to generate an empty .psm1 file if there is some reason for wanting to be able to generate a manifest even when there are no source files. An empty .psm1 can still be imported.
• When there are syntax errors that would prevent the output module from being imported, it would report the errors, including source file and location, and generate no output.

[gaelcolas]

I think those two cases should be looked at separately.

For the no source files, when you build, say a MOF-based DSC resources, you do not require a PSM1/rootmodule.
I have nothing against an empty PSM1, and I think that's actually what we do with most DscResources in the DSC Community. So probably we'd need more information about the error you're facing to troubleshoot (and I'll doublecheck how we're doing it and what we get).

The second point is actually quite complex, because some errors can only be evaluated at runtime, and my be independent of the module itself (i.e. dependency missing, library failing to import, OS version, an so on...).

I'll let @Jaykul share his vision for this tool, but in my opinion that should be covered by tests, and some of them are generic.
This build tool should be the least intrusive possible, and we've seen with Moving Using statements that it's actually very difficult (parsing errors handling is very hard, maybe impossible).

[daviesj]

I was debating whether I should open two threads or one. Ultimately settled on just opening one because I wasn't sure whether the maintainer of this module would even accept the premise that a module which can't even be loaded is invalid output.

My scenario is that I am trying to get an efficient workflow for building & maintaining PowerShell modules. At present that seems to require multiple tools, some related and some not, along with either custom code to cobble everything together or a good memory for command lines and a healthy enjoyment of typing. Whether you are using a build script or manually executing commands for each step, it would help to know of a problem at the earliest possible step. Also, I thought that the build tool would be a logical place to detect/prevent glaring errors in build output which is why I am offering these suggestions.

I don't know a ton about DSC but I looked at the PSDscResources module as an example, and that module's manifest just has RootModule commented out. This would be another good way to handle a case of no source files. However, it would surprise me if there were any situation where having a manifest that specifies RootModule to be a file which doesn't exist would be good.

Regarding the errors, maybe it is a complex issue, but maybe the build tool could still help somewhat. I looked at the code for this module a bit, and I see it is already using the Parser in order to provide the "Moving Using statements" feature you mentioned. Why not at least report the other detected syntax errors? I see that conversion of line numbers from the module file to source is also already implemented so reporting the corresponding source locations would also be easy.

After thinking about it more, I guess that misspelling a keyword (like function) actually makes an invalid command, not a syntax error. I agree that this is a more complex issue. However, it might still be worth considering. I think that the only commands which would make the module fail to import would be outside of class / function declarations. And, I wouldn't expect a typical module should have much of this (maybe set up some variables, load a bundled module or a required assembly). It might work to find all CommandAst's at the root of the final .psm1 script (outside functions / classes) and warn of any that were a simple command (not something like &$expression) but not provided by one of the built-in modules? I would think that would limit the scope enough to provide a good benefit to effort ratio. Just a thought.

[gaelcolas]

any situation where having a manifest that specifies RootModule to be a file which doesn't exist would be good
True, but that's not ModuleBuilder's role, that would be whatever creates that file to start with (template, New-ModuleManifest, user...).

it is already using the Parser in order to provide the "Moving Using statements" feature you mentioned. Why not at least report the other detected syntax errors

Yeah it kinda does warn you about them IF it moved using statements.
https://github.com/PoshCode/ModuleBuilder/blob/master/Source/Private/MoveUsingStatements.ps1#L76-L82

We've actually had to remove failing and warning the users because of #96

Maybe we could move that outside of MoveUsingStatements and always run (or disable those warnings via a switch).

the only commands which would make the module fail to import would be outside of class / function declarations

Nope, a function's parameter using a type that is not available would make that fail for instance. There are other corner cases.
Warning for Parser errors as you said above would be ok by me.

Just so you know, for DSC we use a template system with several tools & tests: https://github.com/gaelcolas/Sampler/tree/master/Sampler
Quick demo here: https://www.youtube.com/watch?v=bbpFBsl8K9k&ab_channel=DSCCommunity

It uses ModuleBuilder, along with other things.
PsDscResources actually does not use this, as it's Microsoft owned.
https://github.com/dsccommunity/SqlServerDsc is an example for DSC resources, and https://github.com/dsccommunity/DscResource.Test is an example of PowerShell module without DscResources.

[daviesj]

any situation where having a manifest that specifies RootModule to be a file which doesn't exist would be good
True, but that's not ModuleBuilder's role, that would be whatever creates that file to start with (template, New-ModuleManifest, user...).

Oops. My mistake. I thought it was ModuleBuilder setting that value. I forgot I had to create something to do that part. In that case you can ignore what I have said about the case with no source files. The followup question that immediately comes to my mind is why ModuleBuilder doesn't set that value when it does create a PSM1 since it is the one choosing the name of the PSM1 file. But maybe you would not be interested in that.

We've actually had to remove failing and warning the users because of #96

Maybe we could move that outside of MoveUsingStatements and always run (or disable those warnings via a switch).

I think that would be helpful.

the only commands which would make the module fail to import would be outside of class / function declarations

Nope, a function's parameter using a type that is not available would make that fail for instance. There are other corner cases.
Warning for Parser errors as you said above would be ok by me.

Not what I meant by command. I meant a cmdlet, alias, native command, etc.. Basically anything that would end up as a CommandAst in the syntax tree. Another way to put it would be the type of thing that Get-Command can get. Maybe there would be a better way to word that but I don't know what it is.

Also, if I understand your example right, I don't think that would cause Import-Module to fail. I suppose it would fail to import if you then call the function from the root of the module (outside of any functions / classes) but then I would definitely call that an edge case. I suppose there would be other things that would not be a parsing error and would cause a failure to import but would be very hard or impossible to detect (like maybe trying to set a module-level variable to an object with an invalid type). This is why I tried to limit the scope of my suggestion to something that might be reasonable to implement: only checking for commands (or whatever you call them) that would be run upon import.

Just so you know, for DSC we use a template system with several tools & tests: https://github.com/gaelcolas/Sampler/tree/master/Sampler
Quick demo here: https://www.youtube.com/watch?v=bbpFBsl8K9k&ab_channel=DSCCommunity

Interesting. I will take a look.

[Jaykul]

For starters, I agree with you -- building a module that won't import and then not failing or at least warning doesn't seem right. I mean, I'm always building a module so that I can run tests against it, so if it won't import, it will fail on the next step of the build anyway.

It used to give up and fail if there were parse errors, but ...

1. Some of these parse errors are purely due to missing types
2. They might be missing just because you haven't loaded the assemblies
3. If ModuleBuilder loads the assemblies (i.e. by importing the module from the output location), it can't unload them

Still, I'd be happier if we were sure it was going to import, so maybe we can:

1. Warn if it can't parse after the changes
2. In that case (only?) import the module with -ErrorAction Stop before returning

---

So far as setting the RootModule, the short version is that manifests are complicated and people didn't agree about what should go in them. Originally, I was modifying the manifest as little as possible, but we've slowly added more features. We could certainly add that one.

[daviesj]

I too am continually building modules in order to be able run tests, as well as in order to use platyPS. I have also begun trying to script some of this to make it quicker.

Still, I'd be happier if we were sure it was going to import, so maybe we can:

1. Warn if it can't parse after the changes

2. In that case (only?) import the module with -ErrorAction Stop before returning

Warning if the PSM1 doesn't parse after moving using statements would catch some things and would be nice. However if you are considering actually importing the module as a test, it might be better to just do that and not bother to report the other parse errors because:

• Import-Module will report parse errors too.
• It would be nice not having a warning if the correct types are all loaded properly by the module.
• Import-Module will fail in cases that are not parse errors. One example that I run into sometimes is a misspelled command or keyword.

If ModuleBuilder loads the assemblies (i.e. by importing the module from the output location), it can't unload them

Yes, that is a concern. It one reason I didn't suggest trying to import the module. However you could try something like $Job = Start-Job { Import-Module } | Wait-Job. I have been doing something like this in my script that loads the module and runs tests and it seems to work. I don't know if it is guaranteed to do so, but for me Start-Job always seems to create a separate process, so any assemblies loaded by the module get unloaded once the job is complete. Then you could check $Job.ChildJobs.Error for any errors and report those (or if you use -ErrorAction Stop, check JobStateInfo instead).

So far as setting the RootModule, the short version is that manifests are complicated and people didn't agree about what should go in them. Originally, I was modifying the manifest as little as possible, but we've slowly added more features. We could certainly add that one.

Yeah, I kind of noticed some of that. That is why once I realized it wasn't ModuleBuilder setting that value in the manifest, I thought you might not be interested. In my opinion, since ModuleBuilder does not always create a .psm1 file, and since when it does it defines the file name, it would be nice if it updated RootModule accordingly.