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

Limit find_packages in setup.py to aiida (sub-)packages. #4204

Merged
merged 5 commits into from
Jun 30, 2020

Conversation

greschd
Copy link
Member

@greschd greschd commented Jun 29, 2020

Fixes #4205

The setuptools.find_packages function finds all packages in the
current directory. This means that docs, tests, and utils
are currently installed when running

python setup.py sdist
pip install dist/aiida-core-<version>.tar.gz

To avoid this, we use the include keyword to only find
aiida and its sub-packages.

The setuptools.find_packages function finds all packages in the
current directory. This means that `docs`, `tests`, and `utils`
are currently installed when running
```bash
python setup.py sdist
pip install dist/aiida-core-<version>.tar.gz
```

To avoid this, we use the `include` keyword to only find
`aiida` and its sub-packages.
@greschd greschd requested a review from ltalirz June 29, 2020 12:51
@codecov
Copy link

codecov bot commented Jun 29, 2020

Codecov Report

Merging #4204 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #4204   +/-   ##
========================================
  Coverage    79.03%   79.03%           
========================================
  Files          467      467           
  Lines        34492    34492           
========================================
  Hits         27259    27259           
  Misses        7233     7233           
Flag Coverage Δ
#django 70.96% <ø> (ø)
#sqlalchemy 71.84% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a32cec...f4c632b. Read the comment docs.

@ltalirz
Copy link
Member

ltalirz commented Jun 29, 2020

I certainly approve this change but I'm not sure what currently the policy is with the release/1.3.0 branch, so let's wait for @sphuber with merging

Remove the following files from the distribution, by removing
them from MANIFEST.in:
- `*.aiida` export files, used for testing
- `*.png` files from the `docs` directory
- files in `utils`, with the exception of `fasentrypoints.py`
  which is used in the install process.
@greschd greschd force-pushed the limit_find_packages branch from f953ffc to 43104e3 Compare June 29, 2020 14:29
The `fastentrypoints` dependency is used to make console scripts
faster, in the case where they are **not** installed as wheels.
If they are installed as wheels, the console scripts are faster
by default.
By specifying `fastentrypoints` as a build requirement in
`pyproject.toml`, we can ensure that it can be imported whenever
the package is installed through `pip`.
This incurs a slight overhead of having to install
`fastentrypoints` even when the package is installed as a wheel,
but we no longer have to vendor `fastentrypoints`.
@greschd greschd force-pushed the limit_find_packages branch from 21afe94 to 6f08bc9 Compare June 29, 2020 18:19
@greschd greschd requested a review from csadorf June 29, 2020 18:20
@greschd
Copy link
Member Author

greschd commented Jun 29, 2020

@csadorf please review the change to utils/dependency_management.py. Confused me for a moment that the pyproject.toml kept being reverted.. especially because I had a second one open, and thought I'd edited the wrong one 😅

Use an absolute link to aiida.net for the logo in the README,
instead of a relative link. This is needed to make the image
work on PyPI.
Remove the now-unused logo file docs/source/logo_aiida_main.png.
@greschd greschd force-pushed the limit_find_packages branch from ca11efb to fe9323e Compare June 29, 2020 20:31
@ltalirz
Copy link
Member

ltalirz commented Jun 29, 2020

You may now also be able to remove this:

-------------------------------------------------------------------------------
fastentrypoints.py Licence:
Copyright (c) 2016, Aaron Christianson
All rights reserved.
Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions are
met:
1. Redistributions of source code must retain the above copyright
notice, this list of conditions and the following disclaimer.
2. Redistributions in binary form must reproduce the above copyright
notice, this list of conditions and the following disclaimer in the
documentation and/or other materials provided with the distribution.
THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED
TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

Since we no longer vendor the `fastentrypoints` code, it can be
removed from the `open_source_licenses.txt` file.
@sphuber
Copy link
Contributor

sphuber commented Jun 30, 2020

@csadorf did you still want to have a look at the change of the dependency_management script?

@csadorf
Copy link
Contributor

csadorf commented Jun 30, 2020

@csadorf did you still want to have a look at the change of the dependency_management script?

Yes, reviewing now.

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

Approving changes to utils/dependency_management.py.

@greschd Any hints as to how we could make that process less confusing? Provide a visible warning during the precommit rewrite or so?

@sphuber sphuber self-requested a review June 30, 2020 08:00
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @greschd

@sphuber sphuber merged commit d582b0f into aiidateam:develop Jun 30, 2020
@greschd
Copy link
Member Author

greschd commented Jun 30, 2020

@csadorf ideal would be a comment in pyproject.toml saying it's auto-generated.. but it seems that's not supported: uiri/toml#77

Since long-term the goal is (probably?) to consolidate configuration of tools like pytest, mypy, etc. into pyproject.toml, I wonder if it's feasible to still auto-generate it then. Or at the least, it'd have to read in the existing file and then just modify the reentry requirement as needed - that's the purpose of auto-generating it, correct?

Of course another option would be just validating the file without auto-fixing, but that's obviously not quite as nice.

@sphuber
Copy link
Contributor

sphuber commented Jun 30, 2020

By the way, I checked this branch on Test PyPI before merging, and everything worked. The logo in the readme worked and the generated script wrapper for verdi had the fast version generated using the fastentrypoints monkey patch

@greschd greschd deleted the limit_find_packages branch June 30, 2020 08:21
@csadorf
Copy link
Contributor

csadorf commented Jul 1, 2020

@csadorf ideal would be a comment in pyproject.toml saying it's auto-generated.. but it seems that's not supported: uiri/toml#77

Since long-term the goal is (probably?) to consolidate configuration of tools like pytest, mypy, etc. into pyproject.toml, I wonder if it's feasible to still auto-generate it then. Or at the least, it'd have to read in the existing file and then just modify the reentry requirement as needed - that's the purpose of auto-generating it, correct?

Of course another option would be just validating the file without auto-fixing, but that's obviously not quite as nice.

@greshd I think auto-generating it every time in this way might have been a poor work-around to ensure its consistency. I will try to improve that next time I have to update that function.

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.

install only aiida package (+ subpackages)
4 participants