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

Fix broken CI, source dir was picked up instead of installed package #472

Merged
merged 1 commit into from
Mar 29, 2025

Conversation

rgommers
Copy link
Collaborator

@rgommers rgommers commented Mar 28, 2025

In addition:

  • Make the pip install . command verbose, so any build warnings show up in CI log - helps with debugging when anything fails.
  • Add comment to check job to explain its purpose better
  • Bump version of cibuildwheel to (latest) 2.23.2
  • Remove macos-12 run, it's deprecated and no longer starts or with a delay, and it was redundant with macos-13.

@rgommers
Copy link
Collaborator Author

Ah wait, the dummy check job is there as a gate for merging, so it actually has a function:

image

I'll put it back and tweak the comment on why it's there.

In addition:

- Make the `pip install .` command verbose, so any build warnings
  show up in CI log - helps with debugging when anything fails.
- Add comment to `check` job to explain its purpose better
- Bump version of cibuildwheel to (latest) 2.23.2
- Remove `macos-12` run, it's deprecated and no longer starts or with a delay,
  and it was redundant with macos-13.
@rgommers
Copy link
Collaborator Author

This fixes CI and is enough to make other PRs mergeable again (the pre-commit failure isn't blocking).

@rdbisme thank you for the commit bit! If you have any preferences on how to use my newly acquired powers, please let me know - in particular, do you want to review all PRs from me, or are you fine with me self-merging PRs that are business as usual, and only wait for your review on things that are actually consequential (e.g., dropping a Python version, adding a new function, etc.)?

pytest --pyargs bottleneck

check:
# This job is here is the "Required" one for merging PRs, and
# it only runs after all the `test` jobs above have run. Hence
# it serves as a check that CI actually ran before a PR gets merged.
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is also another usecase: when you remove jobs (e.g. obsolete python), they stay there "dangling" and you need to remove them manually. If you make the MR depend on this job instead, then you don't need to do that anymore.

@rdbisme
Copy link
Collaborator

rdbisme commented Mar 29, 2025

This fixes CI and is enough to make other PRs mergeable again (the pre-commit failure isn't blocking).

@rdbisme thank you for the commit bit! If you have any preferences on how to use my newly acquired powers, please let me know - in particular, do you want to review all PRs from me, or are you fine with me self-merging PRs that are business as usual, and only wait for your review on things that are actually consequential (e.g., dropping a Python version, adding a new function, etc.)?

Sure. Let's do a couple round trips and then you'll be "free" to go on your own :).

Thanks for taking the time and giving me a hand with this project.

@rdbisme rdbisme merged commit bd6876f into pydata:master Mar 29, 2025
39 of 40 checks passed
@rdbisme
Copy link
Collaborator

rdbisme commented Mar 29, 2025

Also this project historically was holding a sort of (unenforced) semantic commit style for commits. Do we want to keep it? In that case we might introduce pre-commits again.

@rgommers rgommers deleted the fix-ci branch March 29, 2025 19:44
@rgommers
Copy link
Collaborator Author

Sounds good!

Also this project historically was holding a sort of (unenforced) semantic commit style for commits. Do we want to keep it? In that case we might introduce pre-commits again.

I personally avoid pre-commit, too many security risks. If a project I contribute to uses it, I just look at what fails in CI and fix it up.

Happy to use the MNT: etc. prefixes of course - I just missed that that was being used in this repo.

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