-
Notifications
You must be signed in to change notification settings - Fork 284
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
setup.py and CI improvements #645
Conversation
|
||
jobs: | ||
check_duplicate_runs: |
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.
There is an annoying bug of Github Actions - when someone creates a new branch and pushes some commits here, and then creates a pull request, there are 2 copies of workflow running - one for a push event and another for created PR.
This step prevents the second copy of workflow to be run - if tests are already passed for push event then they will be not run twice while PR is created. Also they will not be run if source code hasn't been changed.
exclude docs/_* | ||
|
||
# Examples | ||
graft examples |
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.
There is no such dir in the repo.
global-exclude .git | ||
global-exclude .github |
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.
I've found .github
dir in the .tar.gz of the latest release:
https://files.pythonhosted.org/packages/de/05/6b1809dbe46e21c4018721c14a989a150ff73b4ecf631fe6e22d02cac579/jupyter_client-6.1.12.tar.gz
This is not something that should be included into the package.
|
||
The [Jupyter Contributor Guides](http://jupyter.readthedocs.io/en/latest/contributor/content-contributor.html) provide extensive information on contributing code or documentation to Jupyter projects. The limited instructions below for setting up a development environment are for your convenience. |
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.
404 URL
@@ -0,0 +1,6 @@ | |||
jupyter_core>=4.6.0 |
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.
Let's move all the dependencies to requirements.txt
file - it's easy to update and use instead of running setup.py
each time
@@ -1,5 +0,0 @@ | |||
#!/usr/bin/env python |
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.
Not used since entry_points
item was added into setup.py
@@ -1,11 +1,5 @@ | |||
[flake8] | |||
max-line-length = 100 | |||
|
|||
[bdist_wheel] |
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.
This is a default behavior of bdist_wheel
, no need to set it implicitly
[bdist_wheel] | ||
universal=0 | ||
|
||
[metadata] |
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.
Let's move all the options related to package content to setup.py
pkg_root = pjoin(here, name) | ||
|
||
packages = [] | ||
for d, _, _ in os.walk(pjoin(here, name)): |
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.
There is a find_packages
function with just the same logic
|
||
class bdist_egg_disabled(bdist_egg): |
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.
Nobody uses bdist_egg
in 2021, no need to keep this class in the file.
Thanks a lot @dolfinus ! I would need to look at it in more details, but at first sight this looks great. |
I've rebased this PR to current master branch commit. |
- sphinx>=1.3.6 | ||
- sphinx_rtd_theme | ||
- pip: | ||
- sphinxcontrib_github_alt |
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.
I think the goal of that one is to install the compiled dependencies on readthedocs (pyzmq fails to build otherwise), but we can test see if that works.
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.
Here #521 RTD configuration based on conda environment was replaced with pip based. So it looks like it is not used anymore.
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.
+1 for me,
there are a couple of changes we need to properly document and warn downstream packagers to see if they have objections, but we can do that in further iterations.
+1 for me too, this is a much needed improvement, great job @dolfinus ! |
Thanks a lot @dolfinus! |
I've added several changes to setup.py, Github Actions workflow and other files. You can find the reason of each change below.