8000 Reorganize build files · Issue #6359 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Closed
GaelVaroquaux opened this issue Feb 14, 2016 · 15 comments
Closed

Reorganize build files #6359

GaelVaroquaux opened this issue Feb 14, 2016 · 15 comments
Labels
Build / CI Easy Well-defined and straightforward way to resolve

Comments

@GaelVaroquaux
Copy link
Member

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?
@ogrisel
Copy link
Member
ogrisel commented Feb 18, 2016

I am ok for renaming continuous_integration as build_tools and move cythonize.py there.

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.

@GaelVaroquaux
Copy link
Member Author
GaelVaroquaux commented Feb 18, 2016 via email

@GaelVaroquaux GaelVaroquaux added Easy Well-defined and straightforward way to resolve Need Contributor labels Feb 18, 2016
@GaelVaroquaux
Copy link
Member Author

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.

@nelson-liu
Copy link
Contributor

Can I take a shot at implementing this?

@nelson-liu
Copy link
Contributor

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?

@ogrisel
Copy link
Member
ogrisel commented Feb 18, 2016

yes

@nelson-liu
Copy link
Contributor

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.

@ogrisel
Copy link
Member
ogrisel commented Feb 18, 2016

I think sklearn._build_utils.get_blas_info should stay there for now.

@nelson-liu
Copy link
Contributor

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.

@ogrisel
Copy link
Member
ogrisel commented Feb 18, 2016

You can clean up the useless WindowsError and cython.dat in sklearn/_build_utils/__init__.py while you are at it.

@nelson-liu
Copy link
Contributor

got it, will do 👍

@arthurmensch
Copy link
Contributor

Please raise a PR as soon as you are ready, I'll review it !

@nelson-liu nelson-liu mentioned this issue Feb 18, 2016
Merged
@nelson-liu
Copy link
Contributor

@arthurmensch thanks!

@nelson-liu
Copy link
Contributor

@GaelVaroquaux shall we close this issue?

@GaelVaroquaux
Copy link
Member Author

@nelson-liu : good point. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Build / CI Easy Well-defined and str 3FE4 aightforward way to resolve
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@ogrisel @GaelVaroquaux @arthurmensch @nelson-liu @TomDLT
0