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

make daemon support optional behind a cargo feature #197

Merged
merged 1 commit into from
May 6, 2024

Conversation

stuebinm
Copy link
Contributor

This allows compiling the validator without support for the server/daemon mode by doing cargo build --no-default-features for users who don't intend to run the validator as a service but only want to check e.g. a few local files.

This greatly saves on compile times and binary size (as rough numbers: on my machine I end up with 141 MiB vs 47 MiB for debug builds with and without daemon mode, and 8 MiB vs 3.2 MiB for release builds. Compile times in release mode are less then half without daemon mode, from 1min 23s to just 27s).

This greatly saves on compile times and binary size
(282 vs. 181 crates, and 141 MiB vs 47 MiB for debug builds,
8 MiB vs 3.2 MiB for release builds)
@stuebinm stuebinm requested review from Tristramg, antoine-de and a team as code owners April 15, 2024 19:17
Copy link
Member

@antoine-de antoine-de left a comment

Choose a reason for hiding this comment

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

fine for me, seems reasonable, thanks for the contribution 🙏

@AntoineAugusti is it fine for you? I don't think the PAN would use this and be impacted by this as apart from the read-url thing this should not change anything for you.

It might be nice to add 2 tests using this feature (one starting a server and getting an error, and one validating a file). If you don't feel like it, I can add them if you prefer @stuebinm

Cargo.toml Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
@stuebinm
Copy link
Contributor Author

@antoine-de

It might be nice to add 2 tests using this feature (one starting a server and getting an error, and one validating a file)

hm, i am not sure how to write such a test that depends on a certain feature being not set? (if i add #[cfg(not(feature="daemon"))] to a test, it won't be run at all if default features are enabled, so it seems a little easy to miss it since it usually probably won't be run at all)

@antoine-de
Copy link
Member

yes, you're right, we also need to add cargo test --no-default-features to make sure they will be run 👍
I can do this part if you prefer.

@stuebinm
Copy link
Contributor Author

what's the current state on this? anything i can do to help it along?

@thbar
Copy link
Contributor

thbar commented Apr 22, 2024

what's the current state on this? anything i can do to help it along?

The changes do seem safe for our use on transport.data.gouv.fr (daemon mode). In case we face anything weird, we will come back and open an issue (and we can always deploy a previous commit as a hot-fix if needed).

@antoine-de antoine-de merged commit a7237ae into etalab:master May 6, 2024
1 check passed
@stuebinm stuebinm deleted the daemon-optional branch June 4, 2024 20:19
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.

None yet

4 participants