The Wayback Machine - https://web.archive.org/web/20210711210006/https://github.com/scikit-learn/scikit-learn/pull/18128
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

MAINT add manifest check in circle ci config #18128

Merged
merged 7 commits into from Sep 7, 2020

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

@thomasjpfan thomasjpfan commented Aug 9, 2020

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

@lucyleeow lucyleeow commented Aug 27, 2020

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 nixphix commented Aug 27, 2020

@lucyleeow will update the PR with GH action

.github/workflows/check-manifest.yml Show resolved Hide resolved
MANIFEST.in Outdated Show resolved Hide resolved
@nixphix nixphix marked this pull request as ready for review Sep 1, 2020
@nixphix
Copy link
Contributor Author

@nixphix 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 thomasjpfan 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?

They should be excluded from the maniftest.

@nixphix
Copy link
Contributor Author

@nixphix 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 nixphix:maint/add-manifest-check branch from 33ae62d to 625b47f Sep 4, 2020
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Minor comment, otherwise LGTM

.github/workflows/check-manifest.yml Outdated Show resolved Hide resolved
.github/workflows/check-manifest.yml Outdated Show resolved Hide resolved
@rth
rth approved these changes Sep 7, 2020
Copy link
Member

@rth rth left a comment

Thanks @nixphix !

@rth rth merged commit 3c62364 into scikit-learn:master Sep 7, 2020
21 checks passed
21 checks passed
@lgtm-com
LGTM analysis: C/C++ No code changes detected
Details
@lgtm-com
LGTM analysis: JavaScript No code changes detected
Details
@lgtm-com
LGTM analysis: Python No code changes detected
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
@circleci-artifacts-redirector
ci/circleci: doc artifact Link to 0/doc/_changed.html
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
@codecov
codecov/patch Codecov Report
Details
@codecov
codecov/project Codecov Report
Details
@azure-pipelines
scikit-learn.scikit-learn Build #20200905.4 succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linting) Linting succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux py36_conda_openblas) Linux py36_conda_openblas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux py36_ubuntu_atlas) Linux py36_ubuntu_atlas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux pylatest_pip_openblas_pandas) Linux pylatest_pip_openblas_pandas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux32 py36_ubuntu_atlas_32bit) Linux32 py36_ubuntu_atlas_32bit succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux_Runs pylatest_conda_mkl) Linux_Runs pylatest_conda_mkl succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Windows py36_pip_openblas_32bit) Windows py36_pip_openblas_32bit succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Windows py37_conda_mkl) Windows py37_conda_mkl succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (macOS pylatest_conda_mkl) macOS pylatest_conda_mkl succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (macOS pylatest_conda_mkl_no_openmp) macOS pylatest_conda_mkl_no_openmp succeeded
Details
jayzed82 added 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
Linked issues

Successfully merging this pull request may close these issues.

4 participants