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 bin/naga.rs to a separate crate in workspace #938

Merged
merged 7 commits into from
Jun 12, 2021

Conversation

jakobhellermann
Copy link
Contributor

Splitting this off #934 for a place to discuss this change.

Because of the default-members, running cargo run --features spv-out,wgsl-in input.wgsl output.spv still works.
A disadvantage is that all the cargo features have to be duplicated.

@jakobhellermann jakobhellermann marked this pull request as draft June 2, 2021 17:11
@Gordon-F
Copy link
Collaborator

Gordon-F commented Jun 2, 2021

Maybe we can enable all features for bin target? And remove all feature gates from the bin code. Are there any disadvantages, except compile time?

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

Maybe we can enable all features for bin target? And remove all feature gates from the bin code. Are there any disadvantages, except compile time?

I don't see any, except maybe binary size.

Here's the (approximate) difference in compile speed:

Compile time (debug) Incremental compile time (debug) Compile time (release)
No features 24s 1s 32s
All shader language features 50s 2.3s 110s

@kvark
Copy link
Member

kvark commented Jun 2, 2021

I'm getting warmed up to this path, especially since cargo run would just work (that was the main blocker).
I happen to run the converter a lot, and lately I just stopped bothering with feature selection and switched to --all-features. So following @Gordon-F suggestion to just unconditionally have that, at least for now (we can reconsider later), sounds like a win.

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

We should strongly add env_logger dependency now and uncomment the first line in main()

[[bin]]
name = "naga"
path = "src/main.rs"

[dependencies]
naga = { path = "../" }
naga = { path = "../", features = ["wgsl-in", "wgsl-out", "glsl-in", "glsl-out", "spv-in", "spv-out", "msl-out", "hlsl-out", "dot-out"] }
Copy link
Member

Choose a reason for hiding this comment

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

I wish there was "all_features = true"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The serialize and deserialize features aren't needed anyway. glsl-validate should probably be enabled as well?

Copy link
Member

Choose a reason for hiding this comment

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

we can keep the serialization off now, indeed, but the validation for GLSL should be ON

@@ -288,10 +269,7 @@ fn main() {
}
other => {
let _ = params;
Copy link
Member

Choose a reason for hiding this comment

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

no longer needed (here and in other places)

@kvark
Copy link
Member

kvark commented Jun 3, 2021

Also path = "src/main.rs" isn't needed in the toml, I think

@jakobhellermann jakobhellermann marked this pull request as ready for review June 4, 2021 08:42
@jakobhellermann
Copy link
Contributor Author

Also path = "src/main.rs" isn't needed in the toml, I think

I added the [[bin]] to keep the binary name as naga instead of naga-cli, and removing the path makes cargo error.

Cargo.toml Outdated Show resolved Hide resolved
naga-cli/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

thank you!

@kvark kvark merged commit 4224d14 into gfx-rs:master Jun 12, 2021
Gordon-F pushed a commit to Gordon-F/naga that referenced this pull request Jun 14, 2021
* move bin/naga.rs to a separate crate

* enable all shader languages for naga binary

* [naga-cli] add env logger

* [naga-cli] remove unneccessary code

* [naga-cli]enable glsl-validate feature

* move naga-cli to cli, add trailing newline

* remove commented env_logger dependency
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.

3 participants