Skip to content

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.

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

Open
Flamefire wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Add build_target parameter to PythonPackage #3575

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f59dc40
Add `build_target` parameter to `PythonPackage`
Flamefire Feb 4, 2025
ceefb81
Fix typo
Flamefire Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 23 additions & 13 deletions easybuild/easyblocks/generic/pythonpackage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,9 +387,10 @@ 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). "
Copy link
Contributor

@Micket Micket Feb 12, 2025

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.

Suggested change
'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). "

Copy link
Contributor Author

@Flamefire Flamefire Feb 12, 2025

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.

Copy link
Member

@boegel boegel Feb 12, 2025

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?

"When using setup.py this will be passed to setup.py and defaults to 'build'. "
"When using setup.py it will be auto-generated using the value of 'build_target'. "
"Otherwise it will be used as-is. A value of None then skips the build step. "
"The template %(python)s will be replace by the currently used Python binary.", CUSTOM],
"The template %(python)s will be replaced by the currently used Python binary.", CUSTOM],
'build_target': ['build', "Option to pass to setup.py for building", CUSTOM],
'check_ldshared': [None, 'Check Python value of $LDSHARED, correct if needed to "$CC -shared"', CUSTOM],
'download_dep_fail': [None, "Fail if downloaded dependencies are detected. "
"Defaults to True unless 'use_pip_for_deps' or 'use_pip_requirement' is True.",
Expand All @@ -399,7 +400,7 @@ def extra_options(extra_vars=None):
"(default: ['bin/*'])", CUSTOM],
'install_src': [None, "Source path to pass to the install command (e.g. a whl file)."
"Defaults to '.' for unpacked sources or the first source file specified", CUSTOM],
'install_target': ['install', "Option to pass to setup.py", CUSTOM],
'install_target': ['install', "Option to pass to setup.py for installation", CUSTOM],
'pip_ignore_installed': [True, "Let pip ignore installed Python packages (i.e. don't remove them)", CUSTOM],
'pip_no_index': [None, "Pass --no-index to pip to disable connecting to PyPi entirely which also disables "
"the pip version check. Enabled by default when pip_ignore_installed=True", CUSTOM],
Expand Down Expand Up @@ -784,23 +785,32 @@ def configure_step(self):
run_shell_cmd(cmd % {'python': self.python_cmd}, hidden=True)

def build_step(self):
"""Build Python package using setup.py"""
"""Build Python package using setup.py or pip"""

if get_software_root('CMake'):
include_paths = os.pathsep.join(self.toolchain.get_variable("CPPFLAGS", list))
library_paths = os.pathsep.join(self.toolchain.get_variable("LDFLAGS", list))
env.setvar("CMAKE_INCLUDE_PATH", include_paths)
env.setvar("CMAKE_LIBRARY_PATH", library_paths)

# inject extra '%(python)s' template value before getting value of 'buildcmd' custom easyconfig parameter
self.cfg.template_values['python'] = self.python_cmd
build_cmd = self.cfg['buildcmd']

if self.use_setup_py:

if get_software_root('CMake'):
include_paths = os.pathsep.join(self.toolchain.get_variable("CPPFLAGS", list))
library_paths = os.pathsep.join(self.toolchain.get_variable("LDFLAGS", list))
env.setvar("CMAKE_INCLUDE_PATH", include_paths)
env.setvar("CMAKE_LIBRARY_PATH", library_paths)

# For setup.py-based builds the `buildcmd` was used as the argument to setup.py
# and may even contain options such as: buildcmd = 'build --customize=eb_customize.py'
# But it should be a command to execute, so prepend 'python setup.py' only when it is surely not.
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}"
Comment on lines 804 to +813
Copy link
Contributor

@Micket Micket Feb 12, 2025

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.

Copy link
Contributor Author

@Flamefire Flamefire Feb 12, 2025

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.


if build_cmd:
cmd = ' '.join([self.cfg['prebuildopts'], build_cmd, self.cfg['buildopts']])
Expand Down