-
-
Notifications
You must be signed in to change notification settings - Fork 26.5k
MAINT add manifest check in circle ci config #18128
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
Conversation
|
Since our circleci instances are limited. Can we move this check to azure pipelines or github actions and have it run nightly on master? |
|
Hi @nixphix are you still interested in working on this? If so, aside from @thomasjpfan's comment, it's currently failing due to: I think these should be added to the whitelist in setup.py. |
|
@lucyleeow will update the PR with GH action |
|
@thomasjpfan this is the suggestion from check-manifest now, should I build sklearn to generate these files? or add it to the exclude list? Here you can find the build log missing from VCS:
sklearn/linear_model/_sag_fast.pyx
sklearn/utils/_seq_dataset.pxd
sklearn/utils/_seq_dataset.pyx |
They should be excluded from the maniftest. |
I have added the files to ignore list now the check is passing |
33ae62d to
625b47f
Compare
thomasjpfan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment, otherwise LGTM
rth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nixphix !
Reference Issues/PRs
Fixes #13916
What does this implement/fix? Explain your changes.
Adds manifest check in circle ci set up to check if MANIFEST.in is missing any file.
Any other comments?
Should we run this check as cron job or only on master branch?
Shall I update the
MANIFEST.inwith the suggestion from the check-manifest package?