BLD: exclude Cython generated .c and .cpp files from build artifacts
#4905
Conversation
neutrinoceros
added
build
neutrinoceros
force-pushed
the
build/exclude_generated_code
branch
2 times, most recently
from
d951003
to
69dffdc
Compare
MANIFEST.in
Outdated
| recursive-exclude yt *.cpp *.c | ||
| include yt/utilities/lib/fixed_interpolator.cpp | ||
| include yt/utilities/lib/pixelization_constants.cpp | ||
| include yt/utilities/lib/origami_tags.c | ||
| include yt/utilities/lib/tsearch.c |
There was a problem hiding this comment.
Is there a reason not to include all files found with
$ git ls-files | grep -E '\.[hpp|cpp|c]$'
doc/source/analyzing/_static/axes.c
doc/source/analyzing/_static/axes.h
yt/frontends/artio/artio_headers/artio.c
yt/frontends/artio/artio_headers/artio.h
yt/frontends/artio/artio_headers/artio_endian.c
yt/frontends/artio/artio_headers/artio_endian.h
yt/frontends/artio/artio_headers/artio_file.c
yt/frontends/artio/artio_headers/artio_grid.c
yt/frontends/artio/artio_headers/artio_internal.h
yt/frontends/artio/artio_headers/artio_mpi.c
yt/frontends/artio/artio_headers/artio_mpi.h
yt/frontends/artio/artio_headers/artio_parameter.c
yt/frontends/artio/artio_headers/artio_particle.c
yt/frontends/artio/artio_headers/artio_posix.c
yt/frontends/artio/artio_headers/artio_selector.c
yt/frontends/artio/artio_headers/artio_sfc.c
yt/frontends/artio/artio_headers/cosmology.c
yt/frontends/artio/artio_headers/cosmology.h
yt/geometry/vectorized_ops.h
yt/utilities/lib/cykdtree/windows/stdint.h
yt/utilities/lib/endian_swap.h
yt/utilities/lib/marching_cubes.h
yt/utilities/lib/mesh_triangulation.h
yt/utilities/lib/origami_tags.c
yt/utilities/lib/origami_tags.h
yt/utilities/lib/platform_dep.h
yt/utilities/lib/tsearch.c
yt/utilities/lib/tsearch.hThere was a problem hiding this comment.
I think we should avoid excluding anything that we bundle, and instead focus on just excluding the Cython generated sources. Certainly for sdist these are all necessary.
There was a problem hiding this comment.
Headers are already included by previous instructions (and should be safe since they're never generated as far as I know). I only included the bare minimum set of files that made python -m build run without errors, and overlooked the rest. Now I'm actually not sure why this seems to work, I guess we are not properly testing that sdists we produce are functional.
There was a problem hiding this comment.
In the wheel, I don't think we need the .c and .cpp files since the .so files are included. However, in the source, we definitely need them (have you tried compiling from the mere .tar.gz file?).
There was a problem hiding this comment.
I haven't. I'm thinking we should enable some for of testing in the wheel building workflow first.
Let me draft this for now.
There was a problem hiding this comment.
Ok now we got some form of check rolling, and I addressed this remark (but the grep invoke appeared to be slightly broken so I commited a fixed version of it together with the resulting list)
neutrinoceros
changed the title
.c and .cpp files from build artifacts.c and .cpp files from build artifacts (⏰ wait for #4909)
neutrinoceros
force-pushed
the
build/exclude_generated_code
branch
from
69dffdc
to
0ad57f6
Compare
neutrinoceros
changed the title
.c and .cpp files from build artifacts (⏰ wait for #4909).c and .cpp files from build artifacts
neutrinoceros
force-pushed
the
build/exclude_generated_code
branch
from
0ad57f6
to
1e63a47
Compare
neutrinoceros
force-pushed
the
build/exclude_generated_code
branch
from
7805253
to
83ec7d9
Compare
| @@ -8,9 +8,54 @@ include yt/utilities/mesh_types.yaml | |||
| exclude yt/utilities/lib/cykdtree/c_kdtree.cpp | |||
| prune tests | |||
| prune answer-store | |||
| recursive-include yt *.py *.pyx *.pxi *.pxd *.h *.hpp README* *.txt LICENSE* *.cu | |||
| recursive-include yt *.py *.pyx *.pxi *.pxd README* *.txt LICENSE* *.cu | |||
| recursive-include doc *.rst *.txt *.py *.ipynb *.png *.jpg *.css *.html | |||
| recursive-include doc *.h *.c *.sh *.svgz *.pdf *.svg *.pyx | |||
There was a problem hiding this comment.
Now that we are at it, there are no .svgz nor .pdf in the doc folder, unless they are generated on our CI?
| recursive-include doc *.h *.c *.sh *.svgz *.pdf *.svg *.pyx | |
| recursive-include doc *.h *.c *.sh *.svg *.pyx |
There was a problem hiding this comment.
I think the latex cheatsheet is a generated pdf. I don't know of generated .svgz files but I'd err on the side of caution and label this as out of scope here.
There was a problem hiding this comment.
Fine, this honestly doesn't matter much and is not worth debating as far as I'm concerned :)
| @@ -8,9 +8,54 @@ include yt/utilities/mesh_types.yaml | |||
| exclude yt/utilities/lib/cykdtree/c_kdtree.cpp | |||
There was a problem hiding this comment.
This bit is included below (see line 40).
| exclude yt/utilities/lib/cykdtree/c_kdtree.cpp |
There was a problem hiding this comment.
oh yeah, good catch. I have no idea why but this file is actually empty so it probably doesn't hurt to include it back.
@Xarthisius wrote the exclude line in #3403, so let's give him a chance to comment.
There was a problem hiding this comment.
from that PR's description:
excludes that I added are mostly cosmetic
There was a problem hiding this comment.
OK, whatever you decide I'm happy with. It's not as if adding an empty cpp file would significantly increase the archive size.
PR Summary
This makes wheels and sdists much lighter.
Close #4701
PR Checklist