veracruz-project / veracruz

Main repository for the Veracruz privacy-preserving compute project, an adopted project of the Confidential Compute Consortium (CCC).
https://veracruz-project.com
MIT License
189 stars 39 forks source link

We should check in Cargo.lock files #60

Closed dreemkiller closed 2 years ago

dreemkiller commented 3 years ago

Requested feature We should check in Cargo.lock files to enable repeatable builds Motivation We've had a number of problems arise because dependencies were updated that broke our build process. Checking in the Cargo.lock files would lock us to specific versions, which should get rid of these problems.

Additional context

This isn't really a problem with the dependencies, per se. The dependencies should check for compatibility unless they are incrementing a major release number. However, we are stuck running on a fairly old nightly build. Unless we are able to update to a more recent stable build, it's unreasonable to expect our dependencies to check against our compiler version before releasing.

Thus, checking in our Cargo.lock files feels like the right move. It's possible that, if we ever get on a stable build version, we could change our minds on this.

geky commented 3 years ago

Just an FYI if anyone else sees this error during compilation in the crate value-bag:

error[E0658]: `match` is not allowed in a `const`

This seems to be a nightly/stable versioning issue thing that can be worked around by using Cargo.lock files from a build on another version. I haven't been able to narrow it down further yet.

This proposal would fix this specific build issue.

geky commented 3 years ago

Ah! Narrowed it down to the change from log v0.4.11 -> log v0.4.14

Anyways I'll get a PR with Cargo.lock files up shortly

geky commented 3 years ago

Hmm, I can push up the Cargo.lock files, but what would the real fix be? Fixing the log dependency to "=v0.4.11"?

geky commented 3 years ago

Wait a second, does checking in Cargo.lock files work with different configurations? It looks like the different platforms (SGX/Trustzone/Nitro) have different dependencies, and different Cargo.lock files.

dreemkiller commented 3 years ago

That does create a problem.

Even after we complete #81, sinaloa-test and veracruz-test will have different configurations (unless we want to create platform-specific tests - which we don't). I could see us hacking something together with fancy linking instead of build-time-configuration, but that's the type of thing that ends up being a lot of maintenance.

geky commented 3 years ago

Another idea, after poking around with this value-bag issue, could we instead specify all dependencies as absolute versions ("=v0.4.11")? It would make specifying dependencies a bit more annoying but should in theory give us a repeatable build.

dreemkiller commented 3 years ago

That works for our direct dependencies, but not our vicarious dependencies. If our direct dependencies are not specific in what they depend on, we could still have problems.

Doing that would be better than our current situation, though.

geky commented 3 years ago

Even after we complete #81, sinaloa-test and veracruz-test will have different configurations (unless we want to create platform-specific tests - which we don't).

Hmm, you could put the tests inside a macro, and then expand that macro based on which platform is configured. That would avoid duplication but also let us build for all platforms at once (and create a full set of Cargo.locks)?

dreemkiller commented 3 years ago

Macros are... icky. Especially large macros like that would end up being.

dominic-mulligan-arm commented 3 years ago

That works for our direct dependencies, but not our vicarious dependencies. If our direct dependencies are not specific in what they depend on, we could still have problems.

Doing that would be better than our current situation, though.

Yeah, all of our recent build problems have been related to transitive dependencies fairly deep inside our tree of dependencies that silently update and break compatibility with the version of rustc that we are using. This feels like a missing feature in cargo, namely the ability to pin to a specific compiler version (and crates advertise which compiler versions they work with).

Does anybody have an alternative to @geky's macro idea? I agree that macros are problematic (they slow down compilation times, IDEs struggle with them, etc.) but is there an alternative? This issue is becoming annoying enough that we need to come up with a fix...

dreemkiller commented 3 years ago

Another solution (not great, but ?maybe? I like it more than the macro idea): Check in a different Cargo.lock file for each platform(Cargo.lock.sgx, Cargo.lock.tz, Cargo.lock.nitro). Have the Makefilecopy the platformCargo.lock.*file to Cargo.lock before starting.

dominic-mulligan-arm commented 3 years ago

I think that's a good pragmatic approach for the time being, @dreemkiller. Does anybody have any objections or see any downsides, other than it being a bit of a kludge?

geky commented 3 years ago

To go too far: Symlink Cargo.locks through a directory that is symlinked to the correct set of Cargo.locks?


I'm for this, it's a kludge but I don't see a solution that's not a kludge.

The main difference between multiple locks vs macros in tests is we'd not be compiling everything all the time. This would improve compile times but also push those sort of cross-platform compile errors into CI results.

geky commented 3 years ago

Another thought, Cargo.lock tracks dependencies not features. So if we remove the feature-configured dependencies we should get back to one set of Cargo.locks.

The main culprit seems to be the sgx libraries. Is this something we can compile cross-platform? https://github.com/veracruz-project/veracruz/blob/b9d3aaa8ae0b5fa0c82d6718790466842d3399d2/sinaloa-test/Cargo.toml#L50-L53

dominic-mulligan-arm commented 2 years ago

This is now complete.