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

Remove build.rs? #217

Closed
hdevalence opened this issue Dec 17, 2018 · 7 comments
Closed

Remove build.rs? #217

hdevalence opened this issue Dec 17, 2018 · 7 comments

Comments

@hdevalence
Copy link
Contributor

Right now curve25519-dalek loads itself into the build.rs and bootstraps itself to generate precomputed tables.

This has a few advantages:

  • It's really easy to change the precomputed parameters (only source changes are necessary, instant rebuild)
  • We don't maintain files with giant tables of constants
  • It made it a lot easier to change the 32-bit backend from i32 to u32

It also has downsides:

  • Confusing build process
  • Can't autoselect a backend based on the target architecture
  • Prevents cross-compilation

If we remove the build.rs, we can select the backend on behalf of our users, so that every crate using curve25519-dalek doesn't need to maintain their own feature selectors. Changing the precomputed parameters would be slightly more annoying, because in order to make the change you'd have to first make a test stub that prints the new constants, then copy those into the source file, but this doesn't seem like the biggest deal. Finally, although there aren't giant tables of constants for the serial backends, there are for the vector backend, so the advantages aren't evenly applied already.

Thoughts ? cc @isislovecruft

@tarcieri
Copy link
Contributor

Could you use a different build system than build.rs for managing regenerating those files, e.g. cargo make?

That way the cargo build flow remains simple, but you still have a build system in place to handle regenerating things from scratch.

@isislovecruft
Copy link
Member

What are the issues with cross-compilation? The only one I've hit so far was that cargo-lipo can't be run on a linux machine to produce iOS "universal binaries" and so it has to be compiled from MacOS (but this was fine after I figured it out since I needed to use Xcode regardless).

@hdevalence
Copy link
Contributor Author

I'm not sure if it's the fault of the build.rs exactly, but for instance, when I was working on #215 I couldn't compile the code with -C target_cpu=cannonlake because that affected the build.rs codegen, which then caused a SIGILL on my laptop.

@isislovecruft
Copy link
Member

Oh, that makes sense and sounds annoying. (Out of curiosity, do you know if it was that cargo wasn't passing the same codegen options to both invocations of rustc?)

In any case, I'd be okay with removing build.rs. Particularly if whatever we did instead resulted in a future where we could find some way to specify extra compile-time precomputations, like adding additional basepoint tables. This might be easier as well with only one build stage? And I'm highly in favour of having an automatic way to select the probably-optimal backend per target, and forcing that off only downstream crates.

@hdevalence
Copy link
Contributor Author

(Out of curiosity, do you know if it was that cargo wasn't passing the same codegen options to both invocations of rustc?)

I think the issue was that cargo was passing the same to both, so that the build.rs executable ended up using AVX512 instructions which were not available on the host.

@tarcieri
Copy link
Contributor

Alternatively, could the precomputation produce output in a form other than Rust code you could check in, e.g. a JSON or TOML file? Then you could consume that in build.rs or otherwise and use it to generate code at build time.

That would avoid having to check in the generated code any time you make changes to the generator while also reducing the complexity/responsibilities of build.rs.

@hdevalence
Copy link
Contributor Author

Closed by #293.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants