-
Notifications
You must be signed in to change notification settings - Fork 17
Shrink packaged crate size #53
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
Conversation
license = "MIT" | ||
readme = "./README.md" |
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.
The build
and readme
settings are unnecessary since the files are using default file names.
Cargo.toml
Outdated
"Makefile", "pg_query.h", | ||
"libpg_query/{src,vendor}/**/*.{c,h}", | ||
"libpg_query/protobuf/{pg_query.pb-c.*,pg_query.proto}", | ||
"!libpg_query/src/postgres/*.c", |
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 exclude
field, but it can't be used in conjunction with include
. Instead, exclusion paths have to use !
https://doc.rust-lang.org/cargo/reference/manifest.html#the-exclude-and-include-fields
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.
Can you explain why its okay to exclude those source files? (wouldn't they be needed to build?)
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 just guessed that files not in the include
folder weren't actually needed.
Double checking that, and it seems to be true:
rm libpg_query/src/postgres/*.c
cargo clean
cargo build
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.
Hmm cargo test
seems to fail with a linking error. It's surprising that cargo build
doesn't catch that.
I'll remove this line.
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 practically that only affects these four files:
libpg_query/src/postgres/include/copyfuncs.funcs.c
libpg_query/src/postgres/include/copyfuncs.switch.c
libpg_query/src/postgres/include/equalfuncs.funcs.c
libpg_query/src/postgres/include/equalfuncs.switch.c
And yes, it actually does appear correct that they are not needed (we could probably figure out a way to skip over that when extracting the code in libpg_query). Maybe just to avoid future breakage we could make the rule something like:
!libpg_query/src/postgres/include/{copyfuncs,equalfuncs}.{funcs,switch}.c
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.
Ah, looks like I sent my comment after you commented without seeing your comment - in that case disregard what I said :)
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.
Adding back these files just increased the package size from 1.8 MB to 2.4 MB, so it's not a huge difference. Maybe in the future we should evaluate what files can be removed from the libpg_query repo instead of trying to filter them out here.
crates.io reports the crate size as being fairly large: 4.26 MB (compressed). This PR improves that to 2.4 MB by excluding unneeded files from the cargo package.
To see the list of files included in the package:
To generate a package, and in the output see the resulting package size:
For reference, here's the current list of files. There are probably additional Postgres source files that can be excluded.