Skip to content

Update warning for pyproject.toml license spec#1851

Merged
rmartin16 merged 5 commits intobeeware:mainfrom
rmartin16:license-warning
Jun 3, 2024
Merged

Update warning for pyproject.toml license spec#1851
rmartin16 merged 5 commits intobeeware:mainfrom
rmartin16:license-warning

Conversation

@rmartin16
Copy link
Member

Changes

Notes

  • The presentation of the original warning was a little jarring to me since it spread across my entire terminal. Additionally, though, it wasn't using the "WARNING" banner typically reserved for messages like this.
  • Moreover, I didn't really understand the change I was being asked to make.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@rmartin16 rmartin16 marked this pull request as ready for review May 30, 2024 19:43
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Looks good to me; the Win-3.13 failure is a weird one though. I've seen a that pop up recently, but it's always been transient...

@freakboy3742
Copy link
Member

The Windows failure definitely isn't transient; it's persisted through multiple re-runs, include full reruns after a rebase.

In a desparate search for anything that could be causing this, I noticed that pytest-xdist is installed; I removed it, and the problem went away. I don't know why xdist would be the problem, other than the fact that multi-threading a test suite could cause some interesting cleanup side effects.

Tagging @rmartin16 in case you have strong opinions to the contrary; my inclination is to drop xdist for the test suite if it's going to be more stable.

@rmartin16
Copy link
Member Author

my inclination is to drop xdist for the test suite if it's going to be more stable.

Would tox -e py-fast still work?

Moreover, though, what if you roll back the recent version bump for pytest-xdist? They reworked how process forking works in the latest version.

@freakboy3742
Copy link
Member

my inclination is to drop xdist for the test suite if it's going to be more stable.

Would tox -e py-fast still work?

I guess not - so there'd need to be other cleanups.

Moreover, though, what if you roll back the recent version bump for pytest-xdist? They reworked how process forking works in the latest version.

Well that'd do it... I'll try and see.

@freakboy3742
Copy link
Member

Moreover, though, what if you roll back the recent version bump for pytest-xdist? They reworked how process forking works in the latest version.

Well that'd do it... I'll try and see.

Looks like it doesn't - 3.5.0 generates the same error.

So - my inclination is to drop xdist. While parallel support is nice to have, it doesn't yield that much of a performance benefit; plus, dropping the extra dependency is one less cause of error, and one less dependency to keep up to date. Thoughts?

@rmartin16
Copy link
Member Author

rmartin16 commented Jun 3, 2024

Moreover, though, what if you roll back the recent version bump for pytest-xdist? They reworked how process forking works in the latest version.

Well that'd do it... I'll try and see.

Looks like it doesn't - 3.5.0 generates the same error.

well, my guess is coverage is doing something weird....at least, if my previous investigation showing this unclosed file is coming from coverage is correct.

So - my inclination is to drop xdist. While parallel support is nice to have, it doesn't yield that much of a performance benefit; plus, dropping the extra dependency is one less cause of error, and one less dependency to keep up to date. Thoughts?

I do normally use tox -e py-fast...since it is about 3x faster (although, tox -e py only takes 18s for me...).

One more plea perhaps...what if we just don't install pytest-xdist for 3.13? And if the problem still exists when 3.13 is released, we drop it completely then.

@freakboy3742
Copy link
Member

One more plea perhaps...what if we just don't install pytest-xdist for 3.13? And if the problem still exists when 3.13 is released, we drop it completely then.

I can live with that for now. It's also possible that this might be fixed in 3.13-b2, or with a coverage bump at some point; if we get close to 3.13-final and this is still lingering, we can re-evaluate then.

@freakboy3742
Copy link
Member

@rmartin16 Looks like the py3.13 exclusion on xdist has worked; If you're happy, feel free to merge.

@rmartin16 rmartin16 merged commit fd89e3a into beeware:main Jun 3, 2024
@rmartin16 rmartin16 deleted the license-warning branch June 3, 2024 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants