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.

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

pythonpackage can't return build requirements for wheels, make it return an error if attempted #1852

Merged
merged 1 commit into from Jun 10, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
49 changes: 48 additions & 1 deletion pythonforandroid/pythonpackage.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ def extract_metainfo_files_from_package(
if not os.path.exists(output_folder) or os.path.isfile(output_folder):
raise ValueError("output folder needs to be existing folder")

if debug:
print("extract_metainfo_files_from_package: extracting for " +
"package: " + str(package))

# A temp folder for making a package copy in case it's a local folder,
# because extracting metadata might modify files
# (creating sdists/wheels...)
Expand Down Expand Up @@ -418,6 +422,7 @@ def _extract_metainfo_files_from_package_unsafe(
try:
build_requires = []
metadata_path = None

if path_type != "wheel":
# We need to process this first to get the metadata.

Expand Down Expand Up @@ -447,7 +452,9 @@ def _extract_metainfo_files_from_package_unsafe(
metadata = None
with env:
hooks = Pep517HookCaller(path, backend)
env.pip_install([transform_dep_for_pip(req) for req in build_requires])
env.pip_install(
[transform_dep_for_pip(req) for req in build_requires]
)
reqs = hooks.get_requires_for_build_wheel({})
env.pip_install([transform_dep_for_pip(req) for req in reqs])
try:
Expand All @@ -466,6 +473,15 @@ def _extract_metainfo_files_from_package_unsafe(
"METADATA"
)

# Store type of metadata source. Can be "wheel", "source" for source
# distribution, and others get_package_as_folder() may support
# in the future.
with open(os.path.join(output_path, "metadata_source"), "w") as f:
try:
f.write(path_type)
except TypeError: # in python 2 path_type may be str/bytes:
f.write(path_type.decode("utf-8", "replace"))

# Copy the metadata file:
shutil.copyfile(metadata_path, os.path.join(output_path, "METADATA"))
finally:
Expand Down Expand Up @@ -518,12 +534,23 @@ def _extract_info_from_package(dependency,
- name
- dependencies (a list of dependencies)
"""
if debug:
print("_extract_info_from_package called with "
"extract_type={} include_build_requirements={}".format(
extract_type, include_build_requirements,
))
output_folder = tempfile.mkdtemp(prefix="pythonpackage-metafolder-")
try:
extract_metainfo_files_from_package(
dependency, output_folder, debug=debug
)

# Extract the type of data source we used to get the metadata:
with open(os.path.join(output_folder,
"metadata_source"), "r") as f:
metadata_source_type = f.read().strip()

# Extract main METADATA file:
with open(os.path.join(output_folder, "METADATA"),
"r", encoding="utf-8"
) as f:
Expand All @@ -539,14 +566,34 @@ def _extract_info_from_package(dependency,
raise ValueError("failed to obtain package name")
return name
elif extract_type == "dependencies":
# First, make sure we don't attempt to return build requirements
# for wheels since they usually come without pyproject.toml
# and we haven't implemented another way to get them:
if include_build_requirements and \
metadata_source_type == "wheel":
if debug:
print("_extract_info_from_package: was called "
"with include_build_requirements=True on "
"package obtained as wheel, raising error...")
raise NotImplementedError(
"fetching build requirements for "
"wheels is not implemented"
)

# Get build requirements from pyproject.toml if requested:
requirements = []
if os.path.exists(os.path.join(output_folder,
'pyproject.toml')
) and include_build_requirements:
# Read build system from pyproject.toml file: (PEP518)
with open(os.path.join(output_folder, 'pyproject.toml')) as f:
build_sys = pytoml.load(f)['build-system']
if "requires" in build_sys:
requirements += build_sys["requires"]
elif include_build_requirements:
# For legacy packages with no pyproject.toml, we have to
# add setuptools as default build system.
requirements.append("setuptools")

# Add requirements from metadata:
requirements += [
Expand Down
24 changes: 21 additions & 3 deletions tests/test_pythonpackage_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ def fake_metadata_extract(dep_name, output_folder, debug=False):

Lorem Ipsum"""
))
with open(os.path.join(output_folder, "metadata_source"), "w") as f:
f.write(u"wheel") # since we have no pyproject.toml


def test__extract_info_from_package():
Expand Down Expand Up @@ -87,9 +89,25 @@ def test_get_dep_names_of_package():
dep_names = get_dep_names_of_package("python-for-android")
assert "colorama" in dep_names
assert "setuptools" not in dep_names
dep_names = get_dep_names_of_package("python-for-android",
include_build_requirements=True)
assert "setuptools" in dep_names
try:
dep_names = get_dep_names_of_package(
"python-for-android", include_build_requirements=True,
verbose=True,
)
except NotImplementedError as e:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be tempted to make an IsWheelError subclass or something, on the basis that specific exceptions aren't used often enough, but I'm never really sure about this.

Copy link
Author

@ghost ghost Jun 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@inclement I'm somehow afraid somebody will use that to test if a package is a wheel and then end up with broken code once it is actually implemented and this no longer returns an error (or they use it right but we remove the error so it can no longer be imported once this is implemented and therefore no longer ever thrown, and that breaks their code). So I think I'd rather stick with a NotImplementedError here, personally

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@inclement what are your thoughts on my thought on this? 😛 other than this I think it's ready for merge

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm merging, we can always eventually address that one later if we feel it's critical

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't have approved the PR if I thought this needed changing, sorry if that wasn't clear and thanks for explaining the motivation!

# If python-for-android was fetched as wheel then build requirements
# cannot be obtained (since that is not implemented for wheels).
# Check for the correct error message:
assert "wheel" in str(e)
# (And yes it would be better to do a local test with something
# that is guaranteed to be a wheel and not remote on pypi,
# but that might require setting up a full local pypiserver.
# Not worth the test complexity for now, but if anyone has an
# idea in the future feel free to replace this subtest.)
else:
# We managed to obtain build requirements!
# Check setuptools is in here:
assert "setuptools" in dep_names

# TEST 2 from local folder:
assert "colorama" in get_dep_names_of_package(local_repo_folder())
Expand Down