Closed DMarby closed 7 years ago
mvd-ows
commented
7 years ago Reviewed 3 of 3 files at r1. Review status: all files reviewed at latest revision, 1 unresolved discussion.
.gitignore, line 5 at r1 (raw file):
Cargo.lock /.vscode/ build
/build/ for consistency?
Comments from Reviewable
faern
commented
7 years ago Wow. This adds a ton of complexity. Is it this hard to build statically?
Review status: all files reviewed at latest revision, 4 unresolved discussions.
build-static.sh, line 42 at r1 (raw file):
# Download and compile zlib ZLIB_VERSION="1.2.11" ZLIB_HASH="c3e5e9fdd5004dcb542feda5ee4f0ff0744628baf8ed2dd5d66f8ca1197cb1a1"
What part of all of this requires libz?
build-static.sh, line 44 at r1 (raw file):
ZLIB_HASH="c3e5e9fdd5004dcb542feda5ee4f0ff0744628baf8ed2dd5d66f8ca1197cb1a1" wget https://zlib.net/zlib-1.2.11.tar.gz && \
s/1.2.11/${ZLIB_VERSION}/g ?
build-static.sh, line 61 at r1 (raw file):
# Download and compile openssl OPENSSL_VERSION="1.0.2m" OPENSSL_HASH="8c6ff15ec6b319b50788f42c7abc2890c08ba5a1cdcd3810eb9092deada37b0f"
With this PR: https://github.com/mullvad/oqs-rs/pull/28 We don't depend on OpenSSL any longer. I created that in the hope we can get rid of this part of the script? Plus cut down on build times/complexity/binary size etc.
Comments from Reviewable
faern
commented
7 years ago Review status: all files reviewed at latest revision, 5 unresolved discussions.
.gitignore, line 5 at r1 (raw file):
/build/ for consistency?
Agree.
build-static.sh, line 91 at r1 (raw file):
# Build everything cargo build --release --target=x86_64-unknown-linux-musl
I guess the end goal here is to build the wireguard-* binaries? Thus maybe this script should be inside wireguard-establish-psk/? instead of here? For the same reason we moved build-liboqs.sh down into oqs-sys recently. So we don't fill up the repo root with stuff that is not interesting for people who only want to look at oqs-sys, oqs etc.
Comments from Reviewable
DMarby
commented
7 years ago Rust requires you to build against musl to build statically, so unfortunately that adds quite a bit of complexity, especially if you depend on libraries.
Review status: 1 of 2 files reviewed at latest revision, 5 unresolved discussions.
.gitignore, line 5 at r1 (raw file):
Agree.
Removed as it's not needed anymore, since we're not building the openssl/zlib libraries.
build-static.sh, line 42 at r1 (raw file):
What part of all of this requires libz?
OpenSSL, not needed anymore tho since we got rid of that dependency as well.
build-static.sh, line 44 at r1 (raw file):
s/1.2.11/${ZLIB_VERSION}/g ?
Removed as it's not needed anymore, since we're not building the openssl/zlib libraries.
build-static.sh, line 61 at r1 (raw file):
With this PR: https://github.com/mullvad/oqs-rs/pull/28 We don't depend on OpenSSL any longer. I created that in the hope we can get rid of this part of the script? Plus cut down on build times/complexity/binary size etc.
Removed as it's not needed anymore, since we're not building the openssl/zlib libraries.
build-static.sh, line 91 at r1 (raw file):
I guess the end goal here is to build the `wireguard-*` binaries? Thus maybe this script should be inside `wireguard-establish-psk/`? instead of here? For the same reason we moved `build-liboqs.sh` down into `oqs-sys` recently. So we don't fill up the repo root with stuff that is not interesting for people who only want to look at `oqs-sys`, `oqs` etc.
Done :thumbsup:
Comments from Reviewable
mvd-ows
commented
7 years ago Reviewed 2 of 3 files at r2. Review status: all files reviewed at latest revision, 1 unresolved discussion.
Comments from Reviewable
mvd-ows
commented
7 years ago OK!
Review status: all files reviewed at latest revision, 1 unresolved discussion.
Comments from Reviewable
faern
commented
7 years ago Reviewed 2 of 3 files at r2. Review status: all files reviewed at latest revision, 3 unresolved discussions.
wireguard-establish-psk/build-static.sh, line 25 at r2 (raw file):
# Add rustup target for musl rustup target add x86_64-unknown-linux-musl
I'm usually not fond of scripts that go out and modify my system heavily. What I'm hesitant towards in this script, to various degrees are:
Where I think updating the apt cache and using sudo might be the worst offenders. I might have a broken apt cache or whatever at the moment of running this. Plus, when I run a script as my normal user I don't expect it to become root and mess with my system.
I usually prefer writing the prerequisites in a readme or INSTALL file or similar.
Another benefit of separating prerequisites/setup from actual building is that it would make it easy to run this multiple times to re-build. At the moment every run would update the apt cache and do a lot of other things that I know is already done.
Thoughts on this? Who is supposed to use this script and when?
wireguard-establish-psk/build-static.sh, line 28 at r2 (raw file):
# Build everything cargo build --release --target=x86_64-unknown-linux-musl
This will build with the default toolchain. At least I always have the nightly one as my default. To make sure we don't forget to build releases on the stable toolchain I propose you change this to cargo +stable build ... to make the script use the stable toolchain.
Comments from Reviewable
faern
commented
7 years ago Review status: all files reviewed at latest revision, 4 unresolved discussions.
README.md, line 27 at r2 (raw file):
## Building statically linked binaries In order to build statically linked binaries, a musl toolchain and openssl built for musl is needed.
This text is now outdated since we don't use OpenSSL any longer
Comments from Reviewable
DMarby
commented
7 years ago Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.
README.md, line 27 at r2 (raw file):
This text is now outdated since we don't use OpenSSL any longer
Done.
wireguard-establish-psk/build-static.sh, line 25 at r2 (raw file):
I'm usually not fond of scripts that go out and modify my system heavily. What I'm hesitant towards in this script, to various degrees are: * Adding a rust target without my consent. * Installing apt packages. * Updating the apt cache. * Using sudo. Where I think updating the apt cache and using sudo might be the worst offenders. I might have a broken apt cache or whatever at the moment of running this. Plus, when I run a script as my normal user I don't expect it to become root and mess with my system. I usually prefer writing the prerequisites in a readme or `INSTALL` file or similar. Another benefit of separating prerequisites/setup from actual building is that it would make it easy to run this multiple times to re-build. At the moment every run would update the apt cache and do a lot of other things that I know is already done. Thoughts on this? Who is supposed to use this script and when?
Done. Documented it in the README instead of in the script.
wireguard-establish-psk/build-static.sh, line 28 at r2 (raw file):
This will build with the default toolchain. At least I always have the nightly one as my default. To make sure we don't forget to build releases on the stable toolchain I propose you change this to `cargo +stable build ...` to make the script use the stable toolchain.
Done.
Comments from Reviewable
mvd-ows
commented
7 years ago Reviewed 2 of 2 files at r3. Review status: all files reviewed at latest revision, 3 unresolved discussions.
Comments from Reviewable
faern
commented
7 years ago Reviewed 1 of 2 files at r3. Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.
README.md, line 23 at r3 (raw file):
[`oqs`]: oqs/ [`oqs-kex-rpc`]: oqs-kex-rpc/ [`wireguard-psk-exchange`]: wireguard-psk-exchange/
I realize this reference is now wrong since we renamed that crate. Not introduced in this PR, but since you are tinkering here anyway, could you maybe update it?
README.md, line 25 at r3 (raw file):
[`wireguard-psk-exchange`]: wireguard-psk-exchange/ ## Building statically linked binaries
This header is added after the reference section. The reference section is supposed to be last in the document. I didn't realize until now what file this was actually editing.
As the # Building header above says: "check out their respective documentation". So would this not be better in wireguard-establish-psk/README.md? Since it's specific to only that crate.
Comments from Reviewable
faern
commented
7 years ago Review status: all files reviewed at latest revision, 2 unresolved discussions.
README.md, line 25 at r3 (raw file):
This header is added after the reference section. The reference section is supposed to be last in the document. I didn't realize until now what file this was actually editing. As the `# Building` header above says: "check out their respective documentation". So would this not be better in `wireguard-establish-psk/README.md`? Since it's specific to only that crate.
If not. Then at least make the header state that these instructions are specific to wireguard-establish-psk.
Also the docs reference [build-static.sh] which is now moved. So moving the documentation down would solve that as well.
Comments from Reviewable
DMarby
commented
7 years ago Review status: 1 of 3 files reviewed at latest revision, 2 unresolved discussions.
README.md, line 23 at r3 (raw file):
I realize this reference is now wrong since we renamed that crate. Not introduced in this PR, but since you are tinkering here anyway, could you maybe update it?
Sure thing.
README.md, line 25 at r3 (raw file):
If not. Then at least make the header state that these instructions are specific to `wireguard-establish-psk`. Also the docs reference `[build-static.sh]` which is now moved. So moving the documentation down would solve that as well.
Makes sense to move it, done.
Comments from Reviewable
faern
commented
7 years ago 
Reviewed 2 of 2 files at r4. Review status: all files reviewed at latest revision, all discussions resolved.
Comments from Reviewable
This PR adds a script for producing statically linked binaries.
This change is