-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
Reorganize build files #6359
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
Comments
I am ok for renaming Rather than prefixing |
Rather than prefixing ci scripts with ci_ I would rather put the
install.sh and test_script.sh under a new travis sub-folder as we
already have appveyor and circle sub-folders.
Sounds good. I like this option.
|
For a potential contributor interested in this issue: note that the .travis.yml, circle.yml and appveyor.yml files will need to be adapted to the change of paths. |
Can I take a shot at implementing this? |
Just to verify, if you move sklearn/_build_utils/cythonize.py to a different location, an edit to https://github.com/scikit-learn/scikit-learn/blob/master/setup.py#L180 would have to be made, correct? |
yes |
I'm not sure I understand the function of the init.py files in sklearn/_build_utils/. Should they stay in the folder, or move with cythonize.py? I feel like the former, but again unsure. |
I think |
ok, it seems to be building properly like that. I pushed some changes to my local branch and its being run by circle/travis/appveyor right now. I'll create a PR in the morning if the builds succeed. |
You can clean up the useless |
got it, will do 👍 |
Please raise a PR as soon as you are ready, I'll review it ! |
@arthurmensch thanks! |
@GaelVaroquaux shall we close this issue? |
@nelson-liu : good point. Closing. |
As mentioned in #6354 : the cythonize.py file is really a script, and thus should probably not be in the sklearn directory. It would benefit from a directory of its own.
I propose that we rename the directory "continuous_integration" to "build_tools", maybe rename the various ".sh" files in there to prepend the names with "ci_", as in "ci_push_doc.sh", and move the cythonize.py file in there.
What do people think?
The text was updated successfully, but these errors were encountered: