[install] Refactor install.py.in toward testability

[rpoyner-tri]

Relevant to: #7331

This patch extracts most of the python code from install.py.in to a new
module install_lib.py, and adjusts bazel rules and targets to work with
the new layout.

No actual new tests are added at this step.

The moved code in the new file is only adjusted so that python lint
checks will pass. The new function execute_installation() is
essentially the old main() function, with an extra callable argument.

At this stage, all of the existing tests should pass and existing
install rules should still work. The next step toward closing #7331
would be to increase test coverage of the install_lib.py code.

---

This change is 

[rpoyner-tri]

+@jwnimmer-tri for feature review.

FYI @BetsyMcPhail -- if we should get some more Kitware reviewers in, please add them.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @jwnimmer-tri)

a discussion (no related file):
For a better diff-reading experience, trying pulling the code locally and using git show -C. This will extract only the changed lines in install_lib.py.

---

[jwnimmer-tri]

a discussion (no related file):
Is it important / relevant to still have an .in file with pattern substitutions?

My mental model of this ticket was that we'd have a program install.py that takes a required command-line argument --actions=my_actions.py, with no magic text substitutions anywhere.

---

[rpoyner-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @jwnimmer-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Is it important / relevant to still have an .in file with pattern substitutions?

My mental model of this ticket was that we'd have a program install.py that takes a required command-line argument --actions=my_actions.py, with no magic text substitutions anywhere.

That's possible, but it's also a deeper cut into the .bzl code than I was willing to attempt on the first swing. My reading of the issue was that the bulk of the python code was unavailable for unit test, so opening up that access was what i concentrated on.

If forbidding any calls to ctx.actions.expand_template() is also a goal, we can go around again.

---

[BetsyMcPhail]

Thanks for the heads up, no additional Kitware reviewers needed.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @jwnimmer-tri)

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri and @rpoyner-tri)

a discussion (no related file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

That's possible, but it's also a deeper cut into the .bzl code than I was willing to attempt on the first swing. My reading of the issue was that the bulk of the python code was unavailable for unit test, so opening up that access was what i concentrated on.

If forbidding any calls to ctx.actions.expand_template() is also a goal, we can go around again.

OK Makes sense. I'll review in that context.

---

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK Makes sense. I'll review in that context.

I've filed #16252 to do the bazel reworking first, to lose the template substitution. I'd like to get that merged first, and then revisit this PR. One major test-ability objection is the global variables, and I don't think the actions-library split here will be able to address that on its own. Having the installer be a standalone program should be able to do it.

---

[rpoyner-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I've filed #16252 to do the bazel reworking first, to lose the template substitution. I'd like to get that merged first, and then revisit this PR. One major test-ability objection is the global variables, and I don't think the actions-library split here will be able to address that on its own. Having the installer be a standalone program should be able to do it.

I don't see that the two problems (globals and module structure) are necessarily related, and I'm not a huge fan of exec(), but I'm not particularly a fan of all the __init__.py fiddling in this patch either. By all means, play through.

---

[rpoyner-tri]

Closing -- supplanted by #16252.