Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor install.py.in #7331

Closed
fbudin69500 opened this issue Oct 25, 2017 · 6 comments
Closed

Refactor install.py.in #7331

fbudin69500 opened this issue Oct 25, 2017 · 6 comments
Assignees
@BetsyMcPhail @rpoyner-tri
Labels
component: build system Bazel, CMake, dependencies, memory checkers, linters priority: low

Comments

@fbudin69500
Copy link

install.py.in is not tested. It is getting complex and it is complicated for reviewers to verify that modifications do not break anything.

Fixup methods should be refactored and have unit tests as well as pycodestyle checking. Those function need to turn into *.py code that is imported by the generated :install program(s).

See comments on PR #7301
@fbudin69500 fbudin69500 self-assigned this Oct 25, 2017
@fbudin69500 fbudin69500 mentioned this issue Oct 25, 2017
Merged
@jamiesnape
Copy link
Contributor

Relates #6117.

@jwnimmer-tri jwnimmer-tri mentioned this issue Jan 23, 2018
Merged
@jwnimmer-tri jwnimmer-tri mentioned this issue Nov 14, 2018
Closed
@jwnimmer-tri
Copy link
Collaborator

(Yes; see 6117 for a possible nit to look into as part of this.)

@EricCousineau-TRI
Copy link
Contributor

#10103 might be able to address simplified testing.

@jwnimmer-tri jwnimmer-tri added the component: build system Bazel, CMake, dependencies, memory checkers, linters label Apr 28, 2020
@jwnimmer-tri jwnimmer-tri mentioned this issue Oct 19, 2020
Closed
@rpoyner-tri rpoyner-tri self-assigned this Dec 13, 2021
@rpoyner-tri
Copy link
Contributor

Unless someone else has work-in-progress on this, I will take a look.

rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Dec 14, 2021
@rpoyner-tri
[install] Refactor install.py.in toward testability
7d258d6 
Relevant to: RobotLocomotion#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 RobotLocomotion#7331
would be to increase test coverage of the install_lib.py code.
@rpoyner-tri rpoyner-tri mentioned this issue Dec 14, 2021
Closed
@jwnimmer-tri jwnimmer-tri mentioned this issue Dec 18, 2021
Merged
@jwnimmer-tri
Copy link
Collaborator

In #16252, I moved the install.py.in code into an installer.py binary, which automatically enabled our default linting checks for it.

What remains here is to refactor installer.py for maintainability and test coverage.

rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Jan 20, 2022
@rpoyner-tri
installer: Add tests that invoke by import
199efcd 
Relevant to: RobotLocomotion#7331

This patch doesn't directly improve testability, but it does make it
easier to capture coverage statistics (for example by RobotLocomotion#16392), and gives
some small examples of why globals hinder unit testing.
@rpoyner-tri rpoyner-tri mentioned this issue Jan 20, 2022
Merged
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Jan 20, 2022
@rpoyner-tri
installer: Add tests that invoke by import
9c51388 
Relevant to: RobotLocomotion#7331

This patch doesn't directly improve testability, but it does make it
easier to capture coverage statistics (for example by RobotLocomotion#16392), and gives
some small examples of why globals hinder unit testing.
rpoyner-tri added a commit that referenced this issue Jan 24, 2022
@rpoyner-tri
installer: Add tests that invoke installer main() (#16414)
7ce60a5 
* installer: Add tests that invoke installer main()

Relevant to: #7331

This patch doesn't directly improve testability, but it does make it
easier to capture coverage statistics (for example by #16392), and gives
some small examples of why globals hinder unit testing.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Jan 25, 2022
@rpoyner-tri
tools: Remove global subdirs from installer
1d7af7d 
Relevant to: RobotLocomotion#7331

At best, the subdirs tracking was an optimization; at worst, a
temptation to TOCTOU errors. Removing it is another baby step toward
testability.
@rpoyner-tri rpoyner-tri mentioned this issue Jan 25, 2022
Merged
jwnimmer-tri pushed a commit that referenced this issue Jan 25, 2022
@rpoyner-tri
tools: Remove global subdirs from installer (#16430)
6ebe38b 
Relevant to: #7331

At best, the subdirs tracking was an optimization; at worst, a
temptation to TOCTOU errors. Removing it is another baby step toward
testability.
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Feb 1, 2022
@rpoyner-tri
installer: Add tests that invoke by import
51d6612 
Relevant to: RobotLocomotion#7331

This patch doesn't directly improve testability, but it does make it
easier to capture coverage statistics (for example by RobotLocomotion#16392), and gives
some small examples of why globals hinder unit testing.
@jwnimmer-tri
Copy link
Collaborator

As much as I still don't enjoy the status quo of installer.py code, I think refactoring for testability and maintainability is best done in situ as we fix bugs our add new features, not as a standalone task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@BetsyMcPhail BetsyMcPhail

@rpoyner-tri rpoyner-tri

Labels
component: build system Bazel, CMake, dependencies, memory checkers, linters priority: low
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@fbudin69500 @jamiesnape @BetsyMcPhail @rpoyner-tri @jwnimmer-tri @EricCousineau-TRI