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 three build profiles and infrastructure for their toml config #440

Merged
merged 1 commit into from
Nov 11, 2018

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Nov 6, 2018

Fixes #153
Fixes #160

src/manifest/mod.rs Outdated Show resolved Hide resolved
src/bindgen.rs Outdated Show resolved Hide resolved
src/build.rs Outdated Show resolved Hide resolved
}
BuildProfile::Dev => {
// Plain cargo builds use the dev cargo profile, which includes
// debug info by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

This reminds me that we really should add --dev to Cargo...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that would be nice.

src/manifest/mod.rs Outdated Show resolved Hide resolved
src/manifest/mod.rs Outdated Show resolved Hide resolved
@fitzgen
Copy link
Member Author

fitzgen commented Nov 8, 2018

One of the new tests added depends on this: rustwasm/wasm-bindgen#1020

@fitzgen
Copy link
Member Author

fitzgen commented Nov 8, 2018

This is ready for another round of review.

docs/src/cargo-toml-configuration.md Outdated Show resolved Hide resolved
docs/src/cargo-toml-configuration.md Show resolved Hide resolved
docs/src/cargo-toml-configuration.md Outdated Show resolved Hide resolved
src/manifest/mod.rs Outdated Show resolved Hide resolved
@fitzgen fitzgen force-pushed the build-profiles-toml-config branch 2 times, most recently from 74d1c72 to a9f1ce8 Compare November 9, 2018 22:43
@fitzgen
Copy link
Member Author

fitzgen commented Nov 9, 2018

Ok, I am pretty happy with serde and defaults stuff now. Final round of review?

Copy link
Contributor

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

I think I wasn't clear earlier when you said you wanted to hide the unwraps, but just to confirm you didn't want to take the strategy where there's one struct that has a bunch of non-Option fields and one that has Option and the former implement deserialize in terms of the latter?

Also just to make sure I don't forget, we should file an issue afterwards to handle unknown keys to issue warnings on things like typos. Cargo uses https://crates.io/crates/serde_ignored and wasm-pack can probably use it as well!


/// Get this profile's configured `[wasm-bindgen.debug-js-glue]` value.
pub fn wasm_bindgen_debug_js_glue(&self) -> bool {
self.wasm_bindgen.debug_js_glue.unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I thought you were leaning towards a strategy that removes these unwraps?

Copy link
Member Author

Choose a reason for hiding this comment

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

I leaned back towards them because making the two-struct approach work with nested structs involves a lot of boilerplate and it isn't totally clear how we would know which defaults to use in the nested struct deserialization where we no longer know which profile we are deserializing for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok! Just wanted to confirm. We can always tweak over time if necessary!

@fitzgen
Copy link
Member Author

fitzgen commented Nov 9, 2018

Also just to make sure I don't forget, we should file an issue afterwards to handle unknown keys to issue warnings on things like typos. Cargo uses https://crates.io/crates/serde_ignored and wasm-pack can probably use it as well!

Filed #443

@fitzgen fitzgen merged commit 51e6351 into rustwasm:master Nov 11, 2018
@fitzgen fitzgen deleted the build-profiles-toml-config branch November 11, 2018 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants