Skip to content
This repository has been archived by the owner on Feb 15, 2023. It is now read-only.

MAINT, CI: check _distributor_init.py contents #151

Merged
merged 4 commits into from Nov 28, 2021

Conversation

tylerjereddy
Copy link
Collaborator

  • following on from continued problems reported here:
    Performance issue on macOS arm64 (M1) when installing from wheels (2x libopenblas) scipy/scipy#15050

  • draft in code to check that MacOS wheels get
    the same _distributor_init.py as the one present
    in the wheels repo, rather than the generic nearly-empty
    copy actually present in the 1.7.3 release

  • we've had two pull requests about this and it slipped through into
    a release despite 3 sets of eyes so need to be careful here;
    I'd like to see the test script added here failing for MacOS in CI
    like it does locally, before attempting a fix

  • this is likely still WIP, since relative paths/directories
    can be confusing to predict in the wheels/multibuild control
    flow, which also flushes in and out of containers I think, etc.
    (this probably contributed to the original problem)

  • the script did seem to work in a quick local check though, complaining
    about our new MacOS file inclusion, but happy with the long-standing
    windows wheel inclusion for dirs containing wheels for each OS type:

python verify_init.py /Users/treddy/rough_work/scipy/release_173/release/installers/windows
python verify_init.py /Users/treddy/rough_work/scipy/release_173/release/installers/linux
python verify_init.py /Users/treddy/rough_work/scipy/release_173/release/installers/mac

Traceback (most recent call last):
  File "verify_init.py", line 40, in <module>
    check_for_dist_init(input_dir=input_dir)
  File "verify_init.py", line 34, in check_for_dist_init
    raise ValueError(f"Contents of _distributor_init.py incorrect for
{wheel_file}")
ValueError: Contents of _distributor_init.py incorrect for
/Users/treddy/rough_work/scipy/release_173/release/installers/mac/scipy-1.7.3-cp39-cp39-macosx_10_9_x86_64.whl
  • we could go a step farther here and error out on Linux if the contents of _distributor_init.py did match the copy in the wheels repo, since they should not for that platform, though my primary focus is to be strict on the platforms that do need the adjustments (MacOS/Windows)

* following on from continued problems reported here:
scipy/scipy#15050

* draft in code to check that MacOS wheels get
the same `_distributor_init.py` as the one present
in the wheels repo, rather than the generic nearly-empty
copy actually present in the `1.7.3` release

* we've had two pull requests about this and it slipped through into
a release despite 3 sets of eyes so need to be careful here;
I'd like to the test script added here failing for MacOS in CI
like it does locally, before attempting a fix

* this is likely still wip, since relative paths/directories
can be confusing to predict in the wheels/multibuild control
flow, which also flushes in and out of containers I think, etc.
(this probably contributed to the original problem)

* the script did seem to work in a quick local check though, complaining
about our new MacOS file inclusion, but happy with the long-standing
windows wheel inclusion for dirs containing wheels for each OS type:

`python verify_init.py /Users/treddy/rough_work/scipy/release_173/release/installers/windows`
`python verify_init.py /Users/treddy/rough_work/scipy/release_173/release/installers/linux`
`python verify_init.py /Users/treddy/rough_work/scipy/release_173/release/installers/mac`

```
Traceback (most recent call last):
  File "verify_init.py", line 40, in <module>
    check_for_dist_init(input_dir=input_dir)
  File "verify_init.py", line 34, in check_for_dist_init
    raise ValueError(f"Contents of _distributor_init.py incorrect for
{wheel_file}")
ValueError: Contents of _distributor_init.py incorrect for
/Users/treddy/rough_work/scipy/release_173/release/installers/mac/scipy-1.7.3-cp39-cp39-macosx_10_9_x86_64.whl
```
* try to fix up appveyor paths for init
file verification
@rgommers
Copy link
Contributor

Failures look as expected here. I'll push the fix for inclusion of the file on macOS and see that this turns green.

@rgommers
Copy link
Contributor

The bug was in gh-145, the fix was missed a second /scipy.

@tylerjereddy
Copy link
Collaborator Author

The appveyor fails probably mean I still don't fully grasp the working directory situation there, I suppose we could just delete the check in appveyor for now if we only want to address the immediate issue

@rgommers
Copy link
Contributor

The appveyor fails probably mean I still don't fully grasp the working directory situation there, I suppose we could just delete the check in appveyor for now if we only want to address the immediate issue

That looks like the right order of doing things to me.

@rgommers
Copy link
Contributor

I did look at it, and I don't see an obvious mistake in what you did in appveyor.yml.

@isuruf
Copy link
Contributor

isuruf commented Nov 28, 2021

https://github.com/MacPython/scipy-wheels/blob/master/appveyor.yml#L146 needs to be a copy instead of a move

* adjust appveyor control flow to use a `cp` instead
of `mv` for placing `_distributor_init.py` in the wheel
so that we retain the ability to verify the correct
packaging later on
@tylerjereddy
Copy link
Collaborator Author

good catch, I've pushed the appveyor adjustment in

@tylerjereddy tylerjereddy changed the title WIP, MAINT, CI: check _distributor_init.py contents MAINT, CI: check _distributor_init.py contents Nov 28, 2021
@tylerjereddy
Copy link
Collaborator Author

Ok, CI is perfect now, I'll proceed with merge and backport.

@tylerjereddy tylerjereddy merged commit 14f94fe into MacPython:master Nov 28, 2021
@tylerjereddy tylerjereddy deleted the treddy_verify_init branch November 28, 2021 22:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants