-
Notifications
You must be signed in to change notification settings - Fork 312
Add build_target parameter to PythonPackage
#3575
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
base: develop
Are you sure you want to change the base?
Add build_target parameter to PythonPackage
#3575
Conversation
| @@ -408,17 +408,18 @@ def extra_options(extra_vars=None): | |||
| extra_vars = {} | |||
| extra_vars.update({ | |||
| 'buildcmd': [None, "Command for building the package (e.g. for custom builds resulting in a whl file). " | |||
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.
Please indicate that this is deprecated, e.g.
| 'buildcmd': [None, "Command for building the package (e.g. for custom builds resulting in a whl file). " | |
| 'buildcmd': [None, "DEPRECATED: Command for building the package (e.g. for custom builds resulting in a whl file). " |
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 is not deprecated. It is exactly what it states: If set it will be used for building the package.
There is a deprecated (and undocumented) code path that it uses setup.py {buildcmd} when buildcmd starts with build or build_ext to avoid breaking existing code.
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.
Does it make sense to keep both buildcmd and build_target supported though?
buildcmd clearly leaves room for confusion, so I'm also in favor of deprecating it, and using build_target instead. That should work, no?
| if not build_cmd: | ||
| build_cmd = 'build' # Default value for setup.py | ||
| build_cmd = f"{self.python_cmd} setup.py {build_cmd}" | ||
| build_cmd = f"{self.python_cmd} setup.py {self.cfg['build_target']}" | ||
| elif any(build_cmd.startswith(cmd) for cmd in ('build ', 'build_ext ')) or re.match(r'\w+', build_cmd): | ||
| if ' ' not in build_cmd: | ||
| self.log.deprecated("Use 'build_target' instead of 'buildcmd' " | ||
| "to pass the build target to setup.py", '5.1') | ||
| else: | ||
| self.log.deprecated("Use 'build_target' and 'buildopts' instead of 'buildcmd' " | ||
| "to pass arguments to setup.py", '5.1') | ||
| build_cmd = f"{self.python_cmd} setup.py {build_cmd}" |
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.
I would like to see a more direct discouragement of using buildcmd, now it would slip by without your knowing it was deprecated depending on what you set it to.
grepping through the easyconfigs, i think only PyTorch was treating buildcmd this was, rest was already treating it as build_target, so.. lets just fix PyTorch and forget about that silly case?
if self.cfg['buildcmd'] is not None: # should never be set anymore
build_target = buildcmd
self.log.deprecated('use build target and buildopts instead of buildcmd blablabla')
else:
build_target = buildcmd
...
build_cmd = f'{self.python_cmd} setup.py {build_target}"having it marked clearly deprecated, and the build_target being more clear in what it should contain, I think this is safe enough to change?
You can include a warning on not adding build_opts in there is we wanted, but i'm not sure we need to.
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.
The idea here was to allow using a build command, not removing that. Imagine some Python package that requires ./build_python.sh to create a wheel. See the documentation of the option.
|
Current behavior:
So for 1. it works as intended, only 2 is basically broken/unintuitive. The intention here is:
The We could have easyblocks implement Maybe: Add |
|
@Flamefire I changed to target branch in this PR from |
Using `buildcmd` to pass options to `setup.py` is confusing and makes it impossible to fully replace the build command. Introduce `build_target` similar to `install_target` and deprecate passing a target and optional options via `buildcmd`. The old behavior is used when `buildcmd` is a known setup.py command such as `build` or `build_ext` or a single word, e.g. `clean`.
aa56a6b to
ceefb81
Compare
Using
buildcmdto pass options tosetup.pyis confusing and makes it impossible to fully replace the build command.Introduce
build_targetsimilar toinstall_targetand deprecate passing a target and optional options viabuildcmd.The old behavior is used when
buildcmdis a known setup.py command such asbuildorbuild_extor a single word, e.g.clean.My first attempt was to only detect
buildcmd = '%(python)s ...'as an immediate fix for #3570 but that would limit us to much going forward when a Python package build uses a custom command like./build_wheel --fooIf this is acceptable I can open an accompanying PR to update the EasyConfigs to replace
buildcmd.