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

attempt to fix tox on bcrypt CI #953

Closed
wants to merge 5 commits into from
Closed

Conversation

ngoldbaum
Copy link
Contributor

No description provided.

@alex
Copy link
Member

alex commented Jan 16, 2025

looks like maybe tox.ini needs to be updated?

@ngoldbaum
Copy link
Contributor Author

looks like maybe tox.ini needs to be updated?

I think tox might need an update too now that virtualenv knows about cp313t and will reject cp313 on free-threaded Python? I think --discover might work though...

@ngoldbaum
Copy link
Contributor Author

I think --discover might work though...

That seems to have fixed it. I looked through a few of the jobs and it seems to be discovering the correct python just by finding the first one in $PATH.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Comment on lines 17 to 19
- {VERSION: "3.8", TOXENV: "py38"}
- {VERSION: "3.13", TOXENV: "py313"}
- {VERSION: "3.13t", TOXENV: "py313"}
- {VERSION: "3.13t", TOXENV: "py313t"}
Copy link
Member

Choose a reason for hiding this comment

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

These TOXENVs aren't used anymore, right?

env:
TOXENV: ${{ matrix.PYTHON.TOXENV }}
Copy link
Member

Choose a reason for hiding this comment

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

but without this, the toxenv for mypy, pep8, packaging isn't respected :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, sorry for all the back-and-forth. Should be fixed now, I think.

I also noticed that the information about these environments wasn't showing up in the actions environment name - so I added it back.

@alex
Copy link
Member

alex commented Jan 16, 2025

FYI, I got annoyed enough that I submitted #954 to switch us to nox.

Thanks for poking and debugging this! But assuming nox works, I think that's probably a better direction for us.

@ngoldbaum
Copy link
Contributor Author

Thanks for poking and debugging this! But assuming nox works, I think that's probably a better direction for us.

Fair enough - feel free to merge this if that turns out to have issues. Doesn't look like it though.

@ngoldbaum ngoldbaum closed this Jan 16, 2025
@alex
Copy link
Member

alex commented Jan 16, 2025

Thanks so much for your continued contributions!

What rotten luck that this broke the day after it was merged :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants