Windows fixes

[andrewesweet]

master does not run on Windows (in general) for two reasons:

1. Windows is fussy about creating symlinks. You need to be running with admin privileges, or in Win 10 Developer Mode.
2. Temporary files are being created for Terraform to open (e.g. -out X) and the handles are being held on to by NamedTemporaryDirectory so it can delete the file later. Terraform on Windows does not like this, it wants exclusive access to the file (which is a Windows thing, not a Terraform thing as I understand it).

(1) is resolved by modifying TerraformTest to copy 'extra_files' if we detect we're running on Windows, track successfully copied files, and deleting them in the cleanup routine. This is made overly complex because the existing behaviour cleans up symlinked files that already existed but preserves not-symlinked-files. Since the Windows implementation is now copying files, we have to track successfully copied files separately. If the existing behaviour is in-fact a bug, and only those files symlinked by the setup routine should later be cleaned up, I can simplify the implementation further.

(2) is resolved by in one case by closing the temp files handle immediately, since we only really want a file name that Terraform can write output files to. In the other case, where we actually want the file to exist, so we tell NamedTemporaryDirectory not to track the file for deletion and handle that ourselves.

[google-cla]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[andrewesweet]

@googlebot I signed it!

[andrewesweet]

I tested on Win 10 Pro 20H2 and Ubuntu 20.04 under WSL2.

I see the CI is using cloudbuild, which I'm not familiar with. A bit of Googling reveals there is a community maintained Windows builder. The instructions imply I cannot simply add this into the CI in a PR, but rather there is some setup to do in the GCP project. Would you be amendable to adding Windows checks to the CI in this way, please?

[ludoo]

Hey Andrew, first of all thanks a lot for the PR. Let me see what I can do for the CI part, I have a free day tomorrow and hope to get it done then.

[ludoo]

Andrew, I checked the Windows builder and it seems a bit overkill in terms of maintenance, added costs, and security implications (Cloud Build would essentialy become able to spawn VMs in the project at will). If you're ok with it, I'm fine with having a couple extra tests for Windows that will only be run manually. I don't use Windows anyway. :)

As for the PR, it looks great. My only nit is about the added complexity. Would you be against

• adding a simple method to link files instead of calling os.symlink(link_src, link_dst) in setup, that takes into account the underlying os
• differentiating based on the os in _cleanup()

Unless I'm mistaken, the change should be pretty straightforward.

[andrewesweet]

Another option on the CI could be to use GitHub Actions, but it'd require you to insert an SA key into GitHub Secrets to execute the tests, but I can understand why having another CI mechanism could be unattractive.

I hear you on the complexity. It stems from the current behaviour. cleanup unlinks all symlinks it finds specified in extra_files whether or not those links were created in setup. When we're copying on Windows, I do not think it is appropriate to delete files that were not created by setup. If you agree I can change the existing behaviour so that only symlinks created by setup are removed in cleanup I could simplify the implementation hugely. The filenames parameter of cleanup would become the subset of extra_files that linked by setup, setup would have a simple "if Windows copy else symlink", and cleanup would always be os.unlink. Are you happy for me to proceed on that basis?

[ludoo]

Another option on the CI could be to use GitHub Actions, but it'd require you to insert an SA key into GitHub Secrets to execute the tests, but I can understand why having another CI mechanism could be unattractive.

I have to check if we can use Github Actions, I seem to remember some caveat there. Let me get back to you on this.

I hear you on the complexity. It stems from the current behaviour. cleanup unlinks all symlinks it finds specified in extra_files whether or not those links were created in setup. When we're copying on Windows, I do not think it is appropriate to delete files that were not created by setup. If you agree I can change the existing behaviour so that only symlinks created by setup are removed in cleanup I could simplify the implementation hugely. The filenames parameter of cleanup would become the subset of extra_files that linked by setup, setup would have a simple "if Windows copy else symlink", and cleanup would always be os.unlink. Are you happy for me to proceed on that basis?

Absolutely! And thanks again for this.

[andrewesweet]

Rebased & force pushed new changes as discussed. Much cleaner now.

[ludoo]

Thanks a lot! I'm merging this now, and will then cut a new release and push it on pypi.

As for Windows tests, I'm not enthusiastic about having to spin up a new Windows VM every time tests run, and of the implications of granting compute engine admin permissions to the CI agent. o you think there's a way of concocting a wine container, and running Windows Python in it? That would be super easy to integrate.