-
Notifications
You must be signed in to change notification settings - Fork 339
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
Refactor: Split add_standard_build_actions introducing add_pre_build_commands #1077
Refactor: Split add_standard_build_actions introducing add_pre_build_commands #1077
Conversation
Would be greate, we have the same build problem on windows. |
@@ -207,6 +207,19 @@ def _add_build_actions(cls, executor, context, package, variant, | |||
install_path=install_path | |||
) | |||
|
|||
@classmethod | |||
def _add_pre_build_commands(cls, executor, variant, build_type, |
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.
drop these _add_pre_build_commands
methods and just call add_pre_build_commands
directly.
It's different for _add_build_actions
because those may or may not just comprise of the 'standard' build actions (it's feasible that a specific build system may want to set an env var for eg). However, in the case of pre_build_commands
, there's nothing about that that is specific to a build system.
install, build_path, install_path=None): | ||
"""Perform build actions common to every build system. | ||
|
||
This includes: |
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.
This docstring is wrong, it describes what add_standard_build_actions
used to do.
src/rez/build_system.py
Outdated
@@ -286,8 +286,6 @@ def add_standard_build_actions(cls, executor, context, variant, build_type, | |||
- Setting a standard list on env-vars; | |||
- Executing pre_build_commands(), if the package has one. |
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.
This comment is no longer true
Some minor issues but I should be able to get this merged once they're resolved, cheers |
I corrected what you requested :) |
This PR fixes Issue #974.
Based on the indications from #975 (comment)