regebro / pyroma

Rate your Python packages package friendliness
MIT License
211 stars 24 forks source link

Check if author_email field contains author name (fix #77) #78

Closed bessman closed 1 year ago

bessman commented 2 years ago

This test is a bit naive, and will yield a false positive for emails like: "very.(),:;<>[]\".VERY.\"very@\\ \"very\".unusual"@strange.example.com which is a valid email address (though I'm not sure if pyproject.toml supports it).

Good enough, or a more robust approach needed?

Fix #77

CAM-Gerlach commented 2 years ago

(FYI, you need to include Fix #ISSUENUMBER in the OP, not the title or commit message, to trigger GitHub's linking and autoclose)

bessman commented 2 years ago

Thanks. Let me know if I should squash the commits.

CAM-Gerlach commented 2 years ago

Thanks!

It would also be good, and should be pretty straightforward, to include a test for this, and metadata generated from Setuptools' new pyproject.toml reader in general.

You could just create a new subdir of testdata named e.g. pep621, use ini2toml to convert the setup.cfg in the pep517 directory to pyproject.toml, and then add a new test, e.g. test_pep621, just like test_pep517 except passing pep621 as the directory name instead of pep517.

Thanks. Let me know if I should squash the commits.

Its up to @regebro ; he could also just squash-merge them if he wants.

bessman commented 2 years ago

It would also be good, and should be pretty straightforward, to include a test for this, and metadata generated from Setuptools' new pyproject.toml reader in general.

About that... I should mention that I'm not using setuptools, but rather flit. Even with setuptools version 62, reading all metadata from the .toml [project] table does not seem to work. It works if the build system is set to flit though. Is that acceptable for the test case?

bessman commented 2 years ago

Hmm, the test passes on my end. I'll see what I can do.

bessman commented 2 years ago

Here's the problem, of course: build.BuildBackendException: Backend 'flit_core.buildapi' is not available.

Let's see if the latest commit solves it.

CAM-Gerlach commented 2 years ago

Hmm, well it really shouldn't be needed, since as long as flit_core is specified in the to-be-built package's build-system.requires, the frontend should automatically download and install any required dependencies in the isolated build env. As such, I'm not sure if this issue is due to how the tests are being run, or will be an actual issue at runtime; I haven't had time to dig into that yet. But either way, if there isn't something obvious/straightforward going on, that's probably out of scope here, and this is otherwise LGTM. I'll try to take another look tomorrow, thanks.

hugovk commented 1 year ago

I just ran into this, gentle bump :)

CAM-Gerlach commented 1 year ago

Sorry for loosing track of this!

Upon (finally) giving this "another look", build.ProjectBuilder, which we call .prepare on to get the metadata, just invokes the backend hook in the current environment rather than creating a new isolated one.

However, we should actually be using build.util.project_wheel_metadata(), which does exactly what we manually re-implemented here (builds the metadata with the project's build backend in a temp directory and an isolated env, automatically installing any build-time requirements there, and directly returns the metadata in a MESSAGE object). Pretty sure the only reason I didn't suggest originally was because I was confused by the docs and thought it returned only the WHEEL metadata, not the METADATA metadata (but in fact it returns the latter, which is what we actually want).

In any case, instead of adding flit to extras require (i.e. drop the last commit), you can instead make the following change in pyroma/projectdata.py

diff --git a/pyroma/projectdata.py b/pyroma/projectdata.py
index 4851c65..2775bb9 100644
--- a/pyroma/projectdata.py
+++ b/pyroma/projectdata.py
@@ -1,5 +1,5 @@
 # Extracts information from a project that has a distutils setup.py file.
-import build
+import build.util
 import email
 import email.policy
 import logging
@@ -23,10 +23,7 @@ METADATA_MAP = {

 def get_build_data(path):
-    with tempfile.TemporaryDirectory() as tempdir:
-        metadata_dir = build.ProjectBuilder(str(path)).prepare("wheel", tempdir)
-        with open(pathlib.Path(metadata_dir) / "METADATA", "rb") as metadata_file:
-            metadata = email.message_from_binary_file(metadata_file, policy=email.policy.compat32)
+    metadata = build.util.project_wheel_metadata(path, isolated=True)

     if "Description" not in metadata.keys():
         # Having the description as a payload tends to add two newlines, we clean that up here:

That should get everything working for the tests, and also fix the genuine pyroma issue that it still requires the project's build backend to be installed in the local env (which isn't generally true these days), which this test will also validate.

On another note, it seems I've discovered a new form of MISSINGNO...that's also a legendary Pokemon.

image

regebro commented 1 year ago

Thanks!

CAM-Gerlach commented 1 year ago

FYI, the patch in my above comment both gets the test working without actually needing to have the particular project's backend (flit) installed in the user's env, but more importantly improves several issues with the original approach that I'd suggested for reading project metadata, such that it actually works properly with any PEP 517 build backend, and is simpler, more efficient and more reliable.

I've opened a followup PR, #92 , to implement a refined version of that (which first tries a non-isolated env for efficiency and backward compat, and then falls back to a an isolated env if that fails).