Remove platform-specific conditional dependencies #111

dominic-mulligan-arm · 2021-03-22T16:53:35Z

Remove platform-specific conditional dependencies of the form

[target.'cfg(target_arch = "X")'.dependencies]

from various Cargo.toml files. With the move to support AArch64 and X64 Linux processes as a backend these are no longer appropriate. Instead, adopted optional dependencies selected by feature flags to select backend-specific dependencies.

geky

Looks great to me 👍

veracruz-project-owner · 2021-03-22T18:25:02Z

AWS CodeBuild CI Report

CodeBuild project: Veracruz
Commit ID: b5e625c
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

geky · 2021-03-23T06:03:14Z

I don't think there's a reason to not merge this, but have you considered instead making these use cfg(feature = sgx), etc, directly tying them to the feature flags?

After #81, they may end up not even needing to be conditional.

dominic-mulligan-arm · 2021-03-23T09:17:37Z

I don't think there's a reason to not merge this, but have you considered instead making these use cfg(feature = sgx), etc, directly tying them to the feature flags?

After #81, they may end up not even needing to be conditional.

I was under the impression that does not work in Cargo, see e.g. 1. I'll experiment, though, and use that approach if I can indeed get it to work.

veracruz-project-owner · 2021-03-23T10:51:58Z

AWS CodeBuild CI Report

CodeBuild project: Veracruz
Commit ID: 56d95a9
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

dominic-mulligan-arm · 2021-03-23T11:20:54Z

@geky, selecting target dependencies based on a feature definitely does not work in Cargo. Trying it, you get an explicit warning:

warning: Found feature = ... in target.'cfg(...)'.dependencies. This key is not supported for selecting dependencies and will not work as expected. Use the [features] section instead: https://doc.rust-lang.org/cargo/reference/features.html

Moreover, I think this change is going to be much more involved than I first appreciated. The target_arch, target_env, and target_os are derived from the target triple used for compilation. At the moment, we are compiling for TrustZone with a standard aarch64-unknown-linux-gnu target triple, which does not allow us to distinguish whether we are compiling for TrustZone or plain Linux by examining target_os (which is Linux, in this case). Similarly, we have an analogous issue when compiling for SGX, too.

To properly fix this, we need to follow the approach used in the SDK at the moment, and introduce an OPTEE and SGX-specific target triple, to allow us to distinguish between the different targets (actually, both the Rust OPTEE and SGX SDKs already provide their own triple JSON specification files for this purpose). Unfortunately, I think that this means we will need to use Xargo, instead of Cargo, for compiling the project!

dominic-mulligan-arm · 2021-03-23T12:21:48Z

Thinking more, an alternative would be to drop target-based conditional dependencies altogether, and just use features to select dependencies. Given #81 I think this would actually be the neatest: all platform-specific dependencies are imported by platform-specific crates, which are then selected by other platform-neutral crates using features (in hindsight, this may be what @geky was talking about, above, which I missed).

dreemkiller · 2021-03-23T13:35:18Z

Does anyone remember why we (mostly I...) didn't use xargo to begin with for the TA code?
The example code in the SDK uses Xargo, so I don't recall why we didn't.

dominic-mulligan-arm · 2021-03-23T13:42:16Z

Does anyone remember why we (mostly I...) didn't use xargo to begin with for the TA code?
The example code in the SDK uses Xargo, so I don't recall why we didn't.

Well... having used Xargo in the SDK it's quite fickle, no longer maintained, and very poorly documented. I think not using it, if it wasn't needed at the time, was a pretty defensible decision to make. I think at one point about a year ago I tried adopting Xargo as our main build tool and gave up. I may be more successful this time in using it, with the experience from deploying it in the SDK, but I think we made the right decision in avoiding it early in the project.

ShaleXIONG · 2021-03-23T13:51:23Z

Thinking more, an alternative would be to drop target-based conditional dependencies altogether, and just use features to select dependencies. Given #81 I think this would actually be the neatest: all platform-specific dependencies are imported by platform-specific crates, which are then selected by other platform-neutral crates using features (in hindsight, this may be what @geky was talking about, above, which I missed).

All those packages actually are already guarded by features. I marked many packages as optional and will only been imported when certain features are selected. It will be double assurance by also using (legacy) target-based conditions. However, I do not mind to drop target-based conditions all together.

For @dreemkiller question: when I updated sgx-sdk, I have spent some time on swaping all to xargo. I vaguely remember the interactions between xargo importing tz backend and xargo importing sgx backend causes some problems and then I gave up at some point.

dominic-mulligan-arm · 2021-03-23T14:18:09Z

Thinking more, an alternative would be to drop target-based conditional dependencies altogether, and just use features to select dependencies. Given #81 I think this would actually be the neatest: all platform-specific dependencies are imported by platform-specific crates, which are then selected by other platform-neutral crates using features (in hindsight, this may be what @geky was talking about, above, which I missed).

All those packages actually are already guarded by features. I marked many packages as optional and will only been imported when certain features are selected. It will be double assurance by also using (legacy) target-based conditions. However, I do not mind to drop target-based conditions all together.

For @dreemkiller question: when I updated sgx-sdk, I have spent some time on swaping all to xargo. I vaguely remember the interactions between xargo importing tz backend and xargo importing sgx backend causes some problems and then I gave up at some point.

Does anybody know why some dependencies are guarded by target-dependent conditional clauses in Cargo.toml files, then?

dreemkiller · 2021-03-23T14:34:35Z

Does anybody know why some dependencies are guarded by target-dependent conditional clauses in Cargo.toml files, then?

That's likely legacy cruft. We initially did not use features, instead relying on target information. The features were added later, and the target checks were probably just not removed.

dominic-mulligan-arm · 2021-03-23T14:36:49Z

OK, that's what I was hoping. Then the best way forward seems to be clear: get rid of the target-conditional dependencies and use (optional) features for everything, instead. No need for Xargo, and this will fit nicely with #81.

... over use of target_arch, as with Nitro and upcoming Linux support this is now ambiguous.

veracruz-project-owner · 2021-03-24T09:17:36Z

AWS CodeBuild CI Report

CodeBuild project: Veracruz
Commit ID: 603edfe
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

…go.toml files Removed [target.'cfg(target_env = "XXX")'.dependencies], and similar, from Cargo.toml files in favour of optional dependencies selected by Cargo features.

veracruz-project-owner · 2021-03-24T16:05:51Z

AWS CodeBuild CI Report

CodeBuild project: Veracruz
Commit ID: 71adfaf
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

…server

veracruz-project-owner · 2021-03-24T20:04:23Z

AWS CodeBuild CI Report

CodeBuild project: Veracruz
Commit ID: 0482a21
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

…rom build.rs files Used features instead of querying of the target-triple in build.rs files for runtime-manager and veracruz-server. Removed superfluous dev-dependency from Cargo.toml file for runtime-manager and veracruz-server.

veracruz-project-owner · 2021-03-25T12:58:00Z

AWS CodeBuild CI Report

CodeBuild project: Veracruz
Commit ID: d814e06
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

dominic-mulligan-arm · 2021-03-25T13:57:18Z

+1 reached, merging

dominic-mulligan-arm added build-process Something related to the Veracruz build process always-refactoring A refactoring/code quality change to the Veracruz codebase labels Mar 22, 2021

dominic-mulligan-arm added this to the Veracruz as a Linux process milestone Mar 22, 2021

dominic-mulligan-arm requested review from gbryant-arm and ShaleXIONG March 22, 2021 16:53

geky previously approved these changes Mar 22, 2021

View reviewed changes

dominic-mulligan-arm dismissed geky’s stale review via 56d95a9 March 23, 2021 09:22

dominic-mulligan-arm force-pushed the target-env branch from b5e625c to 56d95a9 Compare March 23, 2021 09:22

gbryant-arm previously approved these changes Mar 23, 2021

View reviewed changes

target-env: prefer target_env and target_os for conditional compilation

603edfe

... over use of target_arch, as with Nitro and upcoming Linux support this is now ambiguous.

dominic-mulligan-arm dismissed gbryant-arm’s stale review via 603edfe March 24, 2021 09:08

dominic-mulligan-arm force-pushed the target-env branch from 56d95a9 to 603edfe Compare March 24, 2021 09:08

target-env: removed target-dependent conditional compilation from Car…

71adfaf

…go.toml files Removed [target.'cfg(target_env = "XXX")'.dependencies], and similar, from Cargo.toml files in favour of optional dependencies selected by Cargo features.

target-env: removed duplicate hex import from Cargo.toml in veracruz-…

0482a21

…server

dominic-mulligan-arm changed the title ~~Prefer target_env and target_os for conditional compilation~~ Remove platform-specific conditional dependencies Mar 25, 2021

ShaleXIONG approved these changes Mar 25, 2021

View reviewed changes

dominic-mulligan-arm merged commit 2bd045b into veracruz-project:main Mar 25, 2021

dominic-mulligan-arm deleted the target-env branch March 25, 2021 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove platform-specific conditional dependencies #111

Remove platform-specific conditional dependencies #111

dominic-mulligan-arm commented Mar 22, 2021 •

edited

Loading

geky left a comment •

edited

Loading

veracruz-project-owner commented Mar 22, 2021

geky commented Mar 23, 2021

dominic-mulligan-arm commented Mar 23, 2021

veracruz-project-owner commented Mar 23, 2021

dominic-mulligan-arm commented Mar 23, 2021

dominic-mulligan-arm commented Mar 23, 2021 •

edited

Loading

dreemkiller commented Mar 23, 2021

dominic-mulligan-arm commented Mar 23, 2021

ShaleXIONG commented Mar 23, 2021 •

edited

Loading

dominic-mulligan-arm commented Mar 23, 2021

dreemkiller commented Mar 23, 2021

dominic-mulligan-arm commented Mar 23, 2021

veracruz-project-owner commented Mar 24, 2021

veracruz-project-owner commented Mar 24, 2021

veracruz-project-owner commented Mar 24, 2021

veracruz-project-owner commented Mar 25, 2021

dominic-mulligan-arm commented Mar 25, 2021

Remove platform-specific conditional dependencies #111

Remove platform-specific conditional dependencies #111

Conversation

dominic-mulligan-arm commented Mar 22, 2021 • edited Loading

geky left a comment • edited Loading

Choose a reason for hiding this comment

veracruz-project-owner commented Mar 22, 2021

AWS CodeBuild CI Report

geky commented Mar 23, 2021

dominic-mulligan-arm commented Mar 23, 2021

veracruz-project-owner commented Mar 23, 2021

AWS CodeBuild CI Report

dominic-mulligan-arm commented Mar 23, 2021

dominic-mulligan-arm commented Mar 23, 2021 • edited Loading

dreemkiller commented Mar 23, 2021

dominic-mulligan-arm commented Mar 23, 2021

ShaleXIONG commented Mar 23, 2021 • edited Loading

dominic-mulligan-arm commented Mar 23, 2021

dreemkiller commented Mar 23, 2021

dominic-mulligan-arm commented Mar 23, 2021

veracruz-project-owner commented Mar 24, 2021

AWS CodeBuild CI Report

veracruz-project-owner commented Mar 24, 2021

AWS CodeBuild CI Report

veracruz-project-owner commented Mar 24, 2021

AWS CodeBuild CI Report

veracruz-project-owner commented Mar 25, 2021

AWS CodeBuild CI Report

dominic-mulligan-arm commented Mar 25, 2021

dominic-mulligan-arm commented Mar 22, 2021 •

edited

Loading

geky left a comment •

edited

Loading

dominic-mulligan-arm commented Mar 23, 2021 •

edited

Loading

ShaleXIONG commented Mar 23, 2021 •

edited

Loading