Skip to content
This repository has been archived by the owner on Jun 8, 2022. It is now read-only.

Windows support #31

Merged
merged 7 commits into from
Jan 7, 2019
Merged

Windows support #31

merged 7 commits into from
Jan 7, 2019

Conversation

raymondbutcher
Copy link
Contributor

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

lorengordon and others added 7 commits January 4, 2019 18:36
Removes python_cmd and shebangs. Now it will just use whatever version of Python it finds in the path. Adds tests for deploying Lambda functions with different Python versions while running different Python versions in the path. The .envrc files are used by Direnv to set the Python version when testing.
This was referenced Jan 4, 2019
@lorengordon
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor

That looks pretty reasonable to me. The custom build script option would probably also address #26 well enough for my use case...

@raymondbutcher
Copy link
Contributor Author

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!

@raymondbutcher raymondbutcher merged commit cdc5696 into master Jan 7, 2019
@raymondbutcher raymondbutcher deleted the windows-support branch January 7, 2019 15:56
@lorengordon
Copy link
Contributor

lorengordon commented Jan 7, 2019

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
Copy link
Contributor Author

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

@raymondbutcher raymondbutcher mentioned this pull request Feb 1, 2019
mbklein pushed a commit to nulib/terraform-aws-lambda that referenced this pull request Apr 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants