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

Build script that does nothing #1259

Merged
merged 4 commits into from
May 18, 2018
Merged

Build script that does nothing #1259

merged 4 commits into from
May 18, 2018

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented May 15, 2018

Eventually we will want a build script that enables Serde impls for i128 and u128 -- #1136. As a first step here is a build script that does nothing to see whether we can roll this out without breaking anyone's workflow, without having a supported feature at stake in the event that it needs to be rolled back.

@dtolnay dtolnay requested a review from oli-obk May 15, 2018 19:42
@dtolnay
Copy link
Member Author

dtolnay commented May 15, 2018

After releasing this I would like to give people a week to complain before we start using this to enable any observable behavior.

@oli-obk
Copy link
Member

oli-obk commented May 15, 2018

Would it be overkill to use the rustc_version crate?

Good idea on the slow rollout. We could probably publish an rc version first when we try out an actual feature.

@dtolnay
Copy link
Member Author

dtolnay commented May 15, 2018

The rustc_version crate adds 2.5 CPU-seconds of compile time on my computer above what this build script adds, which seemed excessive. We don't need a production quality semver parser or complete rustc metadata handling.

Eventually we will want a build script that enables Serde impls for i128
and u128. As a first step here is a build script that does nothing to
see whether we can roll this out without breaking anyone's workflow,
without having a supported feature at stake in the event that it needs
to be rolled back.
@hcpl
Copy link
Contributor

hcpl commented May 16, 2018

FYI there is also the version_check crate which compared to rustc_version is much more lightweight feature-wise and doesn't have dependencies.

@dtolnay
Copy link
Member Author

dtolnay commented May 16, 2018

Nice, I hadn't seen version_check before. That looks a lot more reasonable for our use case.

@dtolnay
Copy link
Member Author

dtolnay commented May 18, 2018

Thanks for demonstrating the implementation with version_check! In my own applications I would definitely use it. But for Serde I am going to stick with some simple parsing in the build script. I believe one of the defining challenges for Serde over the next few years will be avoiding bloat (or the perception of bloat) and there is a vast difference in perception between having dependencies and having no dependencies. I took a look at cargo clean && time cargo build --no-default-features before this PR, with just a build script, and with build script + version_check and the build script by itself adds 9% to compile time which is already close to the limit of what it makes sense to accept for the sake of i128 impls; build script + version_check adds 18%. But let's revisit if this approach turns out unreliable or unmaintainable.

@dtolnay dtolnay merged commit 01b86d5 into master May 18, 2018
@dtolnay dtolnay deleted the build branch May 18, 2018 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants