-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Restore pyroma test for iOS #9116
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
Conversation
|
Not sure what is going on with those three test failures. The PyPy failure seems to be picking up a very old version of PyRoma; the Windows failures seem completely unrelated. |
|
The Windows failures aren't pyroma-related, though (at least, not in this PR) - they're related to test teardown: If the Windows test was previously failing because of Pyroma issues, then I'm guessing that failure was masking the problem with the teardown method. I've added a fix - with any luck, that will fix the Windows failures. |
|
Ok - that's fixed the Windows issues, but not the PyPy stale PyRoma issue... |
|
FYI - I've submitted the "extract metadata from installed package" part of this PR upstream to Pyroma as regebro/pyroma#116 |
| continue | ||
|
|
||
| data[key] = value | ||
| return data |
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.
Should the path be added, as per https://github.com/regebro/pyroma/blob/8ee23ca7c5a8d86bcc2f62f55fc11d89bd72f877/pyroma/projectdata.py#L105?
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 existence of _path is used to drive a check-manifest call. pyroma is resilient to the key not existing.
In the context of reading installed package metadata, you don't have access to the source tree (including version control data) which is necessary for a check-manifest call to succeed.
I'm not convinced that a check-manifest call is adding much in this context, and it can't ever work on iOS (or, looking forward, Android) because we won't have access to the original source directory and VCS metadata. So, adding _path (or an analog) isn't really necessary.
But, if you think it's worth preserving where possible, then we could make the get_data() step conditional on the test platform - on platforms that support subprocesses, we use the full get_data() call; when subprocess isn't available, we use the installed metadata approach (and implicitly skip the check-manifest check). That check becomes a cleaner if/when regebro/pyroma#116 is available in a release.
Restores the Pyroma check on iOS.
Bumping the version of cibuildwheel ensures an updated iOS support package is used; this support package treats the installed dependencies directory as a site package, ensuring that the pyroma module can be discovered by pytest.
The change to the pyroma test itself is required because the
get_data()call requires the use of subprocess - the test builds an entirely fresh wheel, and checks the data of that wheel.The approach proposed here uses importlib to extract the metadata of the wheel that is actually installed for testing purposes (which, really, is the wheel that we should be testing).
Unfortunately, the
map_metadata_keys()method that was available in pyroma 4.3.3 has been removed in 5.0, and internalised into thebuild_metadata()method that uses it. As the implementation is a fairly straightforward key/value transformation, I've opted to replicate that logic.