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

Add preferred default release profile settings #55

Merged
merged 11 commits into from
Jun 16, 2020
Merged

Conversation

ascjones
Copy link
Collaborator

@ascjones ascjones commented Jun 15, 2020

Rel #40, use-ink/ink#448

If any of the following [profile.release] settings are missing, then they are added with the following preferred defaults:

[profile.release]
panic = "abort"
lto = true
opt-level = "z"
overflow-checks = true
codegen-units = 1

Any of the above that are manually specified are not overwritten. This allows manual control over e.g. opt-level where in some cases opt-level = 3 might result in a smaller binary size.

Template changes:

  • Remove [profile.release] section
  • Use scale-info from crates.io instead of type-metadata (reverted: requires ink! 3 release)

@ascjones ascjones requested a review from Robbepop June 15, 2020 11:35
Copy link
Contributor

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

All in all LGTM, some renamings wouldn't hurt though.
Also really nice feature!

src/workspace.rs Show resolved Hide resolved
src/workspace.rs Outdated Show resolved Hide resolved
src/workspace.rs Outdated Show resolved Hide resolved
src/workspace.rs Outdated Show resolved Hide resolved
src/workspace.rs Outdated Show resolved Hide resolved
src/workspace.rs Outdated
Comment on lines 516 to 518
/// False = no LTO
/// True = "Fat" LTO
Bool(bool),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be much worse to have FatLto, ThinLto and Off as variants? Kind of dislike the Bool here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See what you think of the explicit variants

src/workspace.rs Show resolved Hide resolved
src/workspace.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

LGTM

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