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

Migrate to tox 4 #332

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Migrate to tox 4 #332

wants to merge 6 commits into from

Conversation

oerc0122
Copy link
Collaborator

@oerc0122 oerc0122 commented Nov 22, 2024

  • Migrate tox.ini to tox 4
  • Remove custom install from tox.ini
  • Add test for no C extension.
  • Rename phonopy_reader to phonopy-reader for tox support (everywhere needed)
  • Add verbose flag to tox's pip install command

@oerc0122 oerc0122 added the enhancement New feature or request label Nov 22, 2024
@oerc0122 oerc0122 self-assigned this Nov 22, 2024
Copy link
Contributor

github-actions bot commented Nov 22, 2024

Test Results

    26 files   -     7      26 suites   - 7   2h 21m 28s ⏱️ + 30m 44s
 1 066 tests ±    0   1 060 ✅ ±    0   6 💤 ± 0  0 ❌ ±0 
15 695 runs   - 2 325  15 603 ✅  - 2 311  92 💤  - 14  0 ❌ ±0 

Results for commit 4412373. ± Comparison against base commit fb4549d.

♻️ This comment has been updated with latest results.

@oerc0122 oerc0122 force-pushed the 330-tox-4 branch 4 times, most recently from 508f602 to 01de4fa Compare November 22, 2024 23:53
@oerc0122 oerc0122 changed the title Migrate to tox 4 [WIP] Migrate to tox 4 Nov 22, 2024
@oerc0122 oerc0122 force-pushed the 330-tox-4 branch 2 times, most recently from 7ab704e to 2b766be Compare November 25, 2024 11:22
@ajjackson
Copy link
Collaborator

If switching the extra name from phonopy_reader to phonopy-reader works, please make sure to update the references in import error message and installation docs

@oerc0122 oerc0122 force-pushed the 330-tox-4 branch 3 times, most recently from fdadfe9 to e40457a Compare November 25, 2024 14:57
@oerc0122 oerc0122 requested a review from ajjackson November 26, 2024 16:44
@oerc0122 oerc0122 changed the title [WIP] Migrate to tox 4 Migrate to tox 4 Nov 26, 2024
@ajjackson
Copy link
Collaborator

Shall we defer this to post-release, so that things are roughly in sync with #333 ?

@oerc0122
Copy link
Collaborator Author

oerc0122 commented Dec 5, 2024

Shall we defer this to post-release, so that things are roughly in sync with #333 ?

As we discussed in-person the other week, I think this is for the best.

@ajjackson
Copy link
Collaborator

This is looking pretty good except that, bafflingly, the platform constraint is only respected for testenv:py310-minrequirements-linux. py310-no-c (platform = linux: linux) and py310-minrequirements-mac (platform = macos: darwin) are running on every platform.

@ajjackson
Copy link
Collaborator

ajjackson commented Dec 13, 2024

Fixed! This is looking good.

The main concern now is that it requires a different tox version from the release tests, so they can't use the same ci_requirements.txt.

The simplest solution could be to hard-code the tox version in test_release.yml until it is migrated; we don't expect to bump it before it is migrated to tox 4 and using ci_requirements.txt again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants