Skip to content

Conversation

@nixphix
Copy link
Contributor

@nixphix nixphix commented Aug 9, 2020

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.in with the suggestion from the check-manifest package?

@thomasjpfan
Copy link
Member

Since our circleci instances are limited. Can we move this check to azure pipelines or github actions and have it run nightly on master?

@lucyleeow
Copy link
Member

Hi @nixphix are you still interested in working on this?

If so, aside from @thomasjpfan's comment, it's currently failing due to:

missing from VCS:
  sklearn/linear_model/_sag_fast.pyx
  sklearn/utils/_seq_dataset.pxd
  sklearn/utils/_seq_dataset.pyx

I think these should be added to the whitelist in setup.py.

@nixphix
Copy link
Contributor Author

nixphix commented Aug 27, 2020

@lucyleeow will update the PR with GH action

@nixphix nixphix marked this pull request as ready for review September 1, 2020 02:27
@nixphix
Copy link
Contributor Author

nixphix commented Sep 1, 2020

@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

@thomasjpfan
Copy link
Member

@thomasjpfan this is the suggestion from check-manifest now, should I build sklearn to generate these files? or add it to the exclude list?

They should be excluded from the maniftest.

@nixphix
Copy link
Contributor Author

nixphix commented Sep 3, 2020

@thomasjpfan this is the suggestion from check-manifest now, should I build sklearn to generate these files? or add it to the exclude list?

They should be excluded from the maniftest.

I have added the files to ignore list now the check is passing

@nixphix nixphix force-pushed the maint/add-manifest-check branch from 33ae62d to 625b47f Compare September 4, 2020 01:44
Copy link
Member

@thomasjpfan thomasjpfan left a 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

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks @nixphix !

@rth rth merged commit 3c62364 into scikit-learn:master Sep 7, 2020
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
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.

Run check_manifest in one of the CI setups

4 participants