DeterminateSystems / riff

Riff automatically provides external dependencies for Rust projects, with support for other languages coming soon.
https://riff.sh
Mozilla Public License 2.0
486 stars 13 forks source link

Set `hardeningDisable` #20

Closed Hoverbear closed 2 years ago

Hoverbear commented 2 years ago

This is necessary for spirv-tools-sys and other crates which require the build to pass without warning.

linear[bot] commented 2 years ago
DS-228 fsm needs to have `hardeningDisable` configuired

Several crates, such as `spriv-tools-sys` require no warnings on build, so setting `hardeningDisable = [ "fortify" ];` may be required.

cole-h commented 2 years ago

I don't actually know if we want to do this for everything. Having this workaround documented in the README (once that's merged) would be good, but I don't know if I agree with disabling it for every shell...

edolstra commented 2 years ago

If the issue is the use of -Werror, it should be possible to override that by add -Wno-error to NIX_CFLAGS_COMPILE.

Hoverbear commented 2 years ago

Ah let me try that, Eelco!

Hoverbear commented 2 years ago

When I set that it does allow the build to succeed, however the user is faced with several pages of this kind of error:

warning: cargo:warning=                 from /nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/iosfwd:38,      |    ^~~~~~~
warning: cargo:warning=                 from spirv-tools/source/val/validate_barriers.cpp:17:cargo:warning=cargo:warning=                 from /nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/string:38,cargo:warning=                 from spirv-tools/source/extensions.h:18,      |    ^~~~~~~
warning: cargo:warning=
warning: 
warning: 
warning:                  from /nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/x86_64-unknown-linux-gnu/bits/c++config.h:586,                 

image

Is this desired?

Hoverbear commented 2 years ago

@cole-h so do you mean like a fsm shell --yeet option?

cole-h commented 2 years ago

I was thinking more along the lines of "if you get FORTIFY errors when building in debug mode, add ${NIX_HARDENING_ENABLE/fortify} to your environment table", but now that I think about that, that wouldn't work, because we'd need to expand NIX_HARDENING_ENABLE ourselves x)

Maybe something like a --disable-hardening flag that can disable any of the hardening options?

Hoverbear commented 2 years ago

We could also have this become a setting like env (#21 ) or ld (#22 ) and configurable via crate metadata.

lheckemann commented 2 years ago

I think that we should maybe actually disable all hardening (not just fortify) by default. Hardening is of course a good thing, but goes against the grain of what we're trying to achieve with fsm: we want things to build for users with as few extra steps as possible. I'd be more in favour of having the ability to configure specific crates to enable hardening (and doing this extensively in the metadata we ship with fsm), but disabling for anything where we don't specifically know the crate will build with hardening enabled.

Maybe we could automate collecting that data as well (i.e. build with hardening enabled, see if it goes through without warnings, and generate the metadata based on that).

grahamc commented 2 years ago

Are there other crates we've seen fail like this? I wonder if we should wait on this for now, until it is more clear how we should move forward.

Hoverbear commented 2 years ago

I met a few crates that spat out the warnings, and a couple crates (both graphical crates, I can't recall the second one...) that failed hard.

cole-h commented 2 years ago

I'm going to convert this to a draft since we still need to talk more about this, and we probably won't get to this before release.

grahamc commented 2 years ago

I'm going to close this for now and revisit it in the next planning meeting :)