claranet / terraform-aws-lambda

Terraform module for AWS Lambda functions
MIT License
157 stars 126 forks source link

Windows support #31

Closed raymondbutcher closed 5 years ago

raymondbutcher commented 5 years ago

Taking the changes made by @lorengordon in #20 with some minor tweaks to simplify usage and restore Python 2 support.

lorengordon commented 5 years ago

Looks good to me!

Python 3:

.\terraform-aws-lambda\tests\python-versions\python3 [upstream-windows-support ≡]> python --version
Python 3.6.4
.\terraform-aws-lambda\tests\python-versions\python3 [upstream-windows-support ≡]> terraform apply
<...elided...>
Apply complete! Resources: 10 added, 0 changed, 0 destroyed.

Python 2:

.\terraform-aws-lambda\tests\python-versions\python2 [upstream-windows-support ≡]> python --version
Python 2.7.13
.\terraform-aws-lambda\tests\python-versions\python2 [upstream-windows-support ≡]> terraform apply
<...elided...>
Apply complete! Resources: 10 added, 0 changed, 0 destroyed.
raymondbutcher commented 5 years ago

Thanks for checking it so quickly @lorengordon.

In #30 we're adding support for custom build scripts and the python <filename>.py usage we've added here is making it awkward.

@asavoy said:

Are we sure that changing to python build.py is necessary? If Python is correctly installed, build.py will execute properly in Windows. At least that's the case when using the command prompt on my Windows machine.

And looking at the docs...

https://docs.python.org/2.7/using/windows.html says it works with file associations https://docs.python.org/3.5/using/windows.html introduces a launcher tool which supports shebangs too

However in #20 you said it was a problem. What are your thoughts on this? If I removed the python part from the commands would you mind testing it again?

lorengordon commented 5 years ago

@raymondbutcher I'm happy to test it if you want.

However, there are a number of caveats to that approach on Windows. The problem with file associations on Windows is that it requires administrator permissions to create the file association in the first place. That happens automatically in the python installer (or manually after the fact). Once the association is there, it works for any user. But there are rather a lot of contexts and use cases in which the person installing or using python does not have administrator permissions to create the association.

The launcher tool for Windows installs as py.exe, so to use that, would again need something like python_cmd or build_interpreter, and call the build script as py build.py. The launcher does support shebangs, which is nice. And it can work with file associations for the best of both worlds, but, see above.

Also, there are different shells on Windows that all invoke applications slightly differently. In PowerShell, if I just run build.py it gives an error:

build.py : The term 'build.py' is not recognized as the name of a cmdlet, function, script file, or operable program. Check the spelling of the name, or if a path was included, verify that the path is correct and
try again.
At line:1 char:1
+ build.py
+ ~~~~~~~~
    + CategoryInfo          : ObjectNotFound: (build.py:String) [], CommandNotFoundException
    + FullyQualifiedErrorId : CommandNotFoundException

To get that to work in PowerShell, actually need to execute ./build.py (forward or backward slash is fine).

The command shell (cmd) doesn't even seem to work that well. Running either build.py or .\build.py (the forward slash is not valid in the cmd shell), it just keeps trying to open a GUI to let me select which program I want to execute with. Now, once I've selected a program, the next time it does work without prompting (both build.py and .\build.py).

raymondbutcher commented 5 years ago

Thanks for the detailed info. To me it sounds like we might need a var.build_script_interpreter variable that defaults to ["python"], just like the var.python_cmd that you had from the beginning.

module "lambda_built_with_bash" {
  ...
  build_script             = "${path.module}/custom-build.sh"
  build_script_interpreter = [] # rely on the execute permission and shebang, may only work in Unix but that is OK for some projects
  ...
}

module "lambda_built_with_python" {
  ...
  build_script             = "${path.module}/custom-build.py"
  build_script_interpreter = ["python"]  # not necessary to set this as it is the default
  ...
}

module "lambda_built_with_users_python" {
  ...
  build_script             = "${path.module}/custom-build.py"
  build_script_interpreter = ["${var.python_cmd}"]  # project level variable lets windows users pass in `py.exe` or `pythonw.exe` etc for this
  ...
}

I'm also thinking about alternatives like:

module "lambda" {
  ...
  build_command = "make --directory=${path.module}/custom-build"
  build_sources = ["${path.module}/custom-build"]
  ...
}

Then build_sources would be used for the content hash, so if any files in that directory change, it would rebuild the package. This would help with custom build scripts that use multiple files.

lorengordon commented 5 years ago

That looks pretty reasonable to me. The custom build script option would probably also address https://github.com/claranet/terraform-aws-lambda/issues/26 well enough for my use case...

raymondbutcher commented 5 years ago

I'm going to merge this now and figure out the custom build issue in #30. Thanks @lorengordon for your contributions and also your patience!

lorengordon commented 5 years ago

Thanks @raymondbutcher! Do you think #30 will be merged soon? Really just looking for a new tag with this Windows support to target for deployments. :)

raymondbutcher commented 5 years ago

@lorengordon I'm still thinking about the solution for #30. In the meantime, I've tagged v0.10.0 which includes this PR.