alexcrichton / openssl-src-rs

Source code and logic to build OpenSSL from source
Apache License 2.0
68 stars 113 forks source link

Fix building against OpenSSL 3.0.0 (take 2) #104

Closed lupine closed 3 years ago

lupine commented 3 years ago

Unlike OpenSSL 1, OpenSSL 3 requires build.info files in its fuzz/ directory to be present. Without it, the configure script fails. fuzz/ is excluded from the crate for space reasons; this change puts it back and excludes doc/ and demos/ instead, to get back below the 10MiB limit.

More information here: #102

This PR is like https://github.com/alexcrichton/openssl-src-rs/pull/103 but tries to use cargo package to make CI closer to a real build, rather than just rming a bunch of files in the main directory.

Signed-off-by: Nick Thomas me@ur.gs

lupine commented 3 years ago

OK @alexcrichton, I've got a choice between two PRs for you.

Both use exactly the same fix for the actual build - creating empty fuzz/build.info and test/build.info files before running ./Configure, which is sufficient to get it to compile without those directories (so the exclude directives can stay as they are).

Where they differ is in how they modify CI - #103 duplicates the list of excluded files, makes the code directory RW, and rm -rfs those directories prior to running the tests.

This PR (which I prefer) uses cargo package to mirror the contents to target/package/openssl-src-<version> before running the tests. The packaged version lacks the testcrate directory as well as the excluded paths.

It does mean we need to know the version of openssl-src that we're building, to construct the path. This is in Cargo.toml, but how to access it in a way that's sensible across all platforms is a bit of a vexed question :sweat_smile:

It's in cargo metadata, but that requires jq to parse it out, which isn't installed in CI. I didn't fancy trying to work out how to get access to it in windows.

Another option would be to grep | cut | tr the Cargo.toml file. I didn't try this one.

When we cargo run, it's provided as an envvar to the binary that gets built and executed. I took this approach, adding src/bin/output_version.rs. It's a bit weird, but it works.

Or we can just merge #103 :sweat_smile:

sfackler commented 3 years ago

This approach seems cleaner to me as well.

alexcrichton commented 3 years ago

I agree that this is better than hardcoding lists, but build scripts ideally shouldn't be modifying the contents of the directories that they're unpackaged into. I'm also wary to delete parts of OpenSSL's source tree that it actually needs to build and trying to fake our way to adding them back in.

Are there perhaps more targeted exclusions of files which don't affect the build system but still allow us to get the tarball under the 10MB limit?

sfackler commented 3 years ago

We could also ask Crates.io to bump the limit for us. It seems like the official tarball off of openssl.org is ~14mb as well.

lupine commented 3 years ago

@alexcrichton hmm, I note that until just recently we were modifying the source code directly, not just the build system ;)

However, I've worked out that we can actually make test/ unnecessary, by using no-tests (instead of the former no-unit-tests). There's no way exclude fuzz/ that I can see, but leaving out test/ gets us back down to 11MiB.

I've just pushed a commit that restores fuzz/ and takes out demos/ and doc/, which gets us down to below 10MiB here. Let's see if CI passes with it.

lupine commented 3 years ago

Since we're all agreed that the CI changes are best like this, I'll close #103

lupine commented 3 years ago

OK, this looks like it would do the trick @alexcrichton . WDYT?

openssl-src-rs$ du -bs target/package/*
60262607    target/package/openssl-src-300.0.0+3.0.0
9800148 target/package/openssl-src-300.0.0+3.0.0.crate
alexcrichton commented 3 years ago

Thanks, looks good to me!