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

Move cargo release configuration to CI #384

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

e00E
Copy link
Contributor

@e00E e00E commented Mar 1, 2022

By setting these in Cargo.toml everyone is forced to build with them. It
is better to leave the defaults to Cargo and the user building the
project. If there are important gains in binary size or speed with
these options they can continue to be set for the official builds as
performed by CI.

By setting these in Cargo.toml everyone is forced to build with them. It
is better to leave the defaults to Cargo and the user building the
project. If there are important gains in binary size or speed  with
these options they can continue to be set for the official builds as
performed by CI.
Copy link
Owner

@osa1 osa1 left a comment

Choose a reason for hiding this comment

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

I'm not sure about this. I had problems with this in other projects in the past. If I'm running make release, cargo build --release, or a similar command that from the look of it seems like it should produce a release build (i.e. a build that could be distributed as a new release), then it should produce a release build. I find it frustrating when I need to set a bunch of env variables, or touch files etc. to generate builds for releasing.

I understand that in many projects you need to set target-specific flags or env variables etc. but in tiny this is not the case and we can keep the build steps simple, as it is the case currently.

If we still want to make this change, we should provide a script that will allow generating release builds with just one command, and use that script in CI. Devs should not have to do searching in the repo to find out how to make builds identical to the ones being released.

Secondly, at least in my workflow, I don't need --release to build fast. When working on tiny I use cargo watch, cargo check, or cargo build. Using build without --release is a better idea for development for other reasons too: it provides better backtraces, it enables debug assertions and overflow/underflow checking in arithmetic operators etc.

So I'm not sure if the motivation here is strong enough. Being able to easily build the project is important too.

@e00E
Copy link
Contributor Author

e00E commented Mar 2, 2022

There are two motivations for this change.

  1. I feel there is not sufficient reason to move away from whatever cargo thinks a release build should be. For every compiler option that is changed from the defaults there better had be a good reason.

  2. Whoever is creating a release build might have their own opinion on how they want to optimize. For example if I am building a lot of rust programs maybe I don't want fat lto or only 1 codegen units because it would be too slow. Or if I am building for an embedded device maybe I want to optimize for size.
    This is easier to achieve when projects do not change these settings. It is not clear that the project should make this decision for the person releasing unless there are extreme circumstances like you benchmarked and saw that you really need lto, otherwise the program would be half as fast.

For 1. we could benchmark. There is probably no significant speed difference and I don't know how to measure it. I think I saw the binary size change between 2.7 and 2.9 MB on my machine.

I've made my arguments so feel free to close the PR if you still disagree. I don't think it's a huge problem to keep these options either, just felt like a slight improvement.

@osa1
Copy link
Owner

osa1 commented Mar 2, 2022

I feel there is not sufficient reason to move away from whatever cargo thinks a release build should be.
...
For 1. we could benchmark. There is probably no significant speed difference and I don't know how to measure it. I think I saw the binary size change between 2.7 and 2.9 MB on my machine.

Runtime performance does not matter too much as tiny should be spending 99.99% of the time waiting for IO events (from IRC connections, or key inputs), and when an IO event appears I don't think the updates we do take significant enough time to require optimizations. I don't have any numbers, but I would be surprised if an event takes more than a millisecond for tiny to process.

However binary size difference is quite significant with LTO. I just compared master branch with and without the [profile.release] setting, using the default features. Generated binary sizes:

  • With LTO: 5,734,904 bytes
  • Without LTO: 8,264,752 bytes
  • Difference: +2,529,848 bytes, +44.1%

That's quite significant.

Whoever is creating a release build might have their own opinion on how they want to optimize

This is a good point. I think it's not possible to override profile.release settings from the command line, right? So if someone wants to override release settings they will have to edit the file. That's a bit inconvenient.

Let me think about this a little bit more.. Please feel free to add if you have any more arguments or thoughts :-)

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.

2 participants