-
Notifications
You must be signed in to change notification settings - Fork 394
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.
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
Minor build cleanups #10007
Minor build cleanups #10007
Conversation
@Myoldmopar @Myoldmopar it has been 28 days since this pull request was last updated. |
2 similar comments
@Myoldmopar @Myoldmopar it has been 28 days since this pull request was last updated. |
@Myoldmopar @Myoldmopar it has been 28 days since this pull request was last updated. |
@Myoldmopar it has been 21 days since this pull request was last updated. |
@Myoldmopar it has been 7 days since this pull request was last updated. |
@Myoldmopar it has been 17 days since this pull request was last updated. |
@Myoldmopar it has been 7 days since this pull request was last updated. |
@Myoldmopar it has been 7 days since this pull request was last updated. |
@Myoldmopar it has been 8 days since this pull request was last updated. |
@Myoldmopar it has been 7 days since this pull request was last updated. |
@Myoldmopar it has been 8 days since this pull request was last updated. |
@Myoldmopar it has been 18 days since this pull request was last updated. |
@Myoldmopar it has been 7 days since this pull request was last updated. |
@Myoldmopar it has been 11 days since this pull request was last updated. |
@Myoldmopar it has been 8 days since this pull request was last updated. |
@Myoldmopar it has been 7 days since this pull request was last updated. |
1 similar comment
@Myoldmopar it has been 7 days since this pull request was last updated. |
@Myoldmopar it has been 13 days since this pull request was last updated. |
@Myoldmopar it has been 8 days since this pull request was last updated. |
1 similar comment
@Myoldmopar it has been 8 days since this pull request was last updated. |
@Myoldmopar it has been 9 days since this pull request was last updated. |
@Myoldmopar it has been 7 days since this pull request was last updated. |
@Myoldmopar it has been 12 days since this pull request was last updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are extremely minimal changes now that I took out the flow lock stuff. Just a few Python warnings fixed.
|
||
def find_libs(dirPath): | ||
def find_libs(dir_path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pythonic_variable_naming
shutil.copytree(standard_lib_dir, target_dir) | ||
|
||
# On Windows, we also need to grab the DLLs folder, which is one folder up | ||
if platform.system() == 'Windows': | ||
python_root_dir = os.path.dirname(standard_lib_dir) | ||
dll_dir = os.path.join(python_root_dir, 'DLLs') | ||
copy_tree(dll_dir, target_dir) | ||
shutil.copytree(dll_dir, target_dir, dirs_exist_ok=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use shutil
version of copytree instead of the distutils
imported version of it.
@@ -213,13 +213,13 @@ def main(): | |||
file_contents = f.read() | |||
|
|||
# first warn about commented lines | |||
p = re.compile('//\s*SetupOutputVariable') | |||
p = re.compile(r'//\s*SetupOutputVariable') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PEP was complaining about the escapes in these, just tell Python they are raw string literals and it won't investigate the escapes.
I'm going to kick off a test package build to see how it goes. Usually when I wait a while between versions, Qt gets updated and we have to tweak the Windows build rules, so I thoroughly expect that here :) |
All builds were successful with these changes, no issues. Merging this. |
Pull request overview
I've had a task on my todo list to clean out the FlowLock logic from plant component models. For most models, they shouldn't operate based on that flag. They should figure out how much flow they want, ask the plant for it, and then use that much flow to calculate the resulting control and physics. Their behavior shouldn't depend on whether the plant is currently unlocked or not. This will cause diffs, but I am curious about whether it's better now. I'll need to generate some plots to evaluate once results start coming out.