Skip to content

Fix the build error with "with-serde" feature in CH branch (on AArch64) #1

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

Merged
merged 1 commit into from
Jul 31, 2020

Conversation

michael2012z
Copy link
Member

@michael2012z michael2012z commented May 14, 2020

This PR is the dependency of cloud-hypervisor/cloud-hypervisor#1168.

Some code for supporting Serde was introduced on AArch64. But the
support was incomplete, some errors are seen while building on ARM.
This commit reverts all Serde support for AArch64. So nothing is
different even if "with-serde" feature is enabled on ARM.
This is a workaround for build before Serde is ready on ARM.

The change was verified on X86 and ARM with a testing PR
michael2012z/cloud-hypervisor#22.

@michael2012z
Copy link
Member Author

Hi, @rbradford, @sboeuf PTAL.

@sboeuf
Copy link
Member

sboeuf commented May 14, 2020

@andreeaflorescu @aghecenco would you mind having a quick look? Did you run into the same issues since I think you maintain aarch64 support on your side too.

@andreeaflorescu
Copy link

I don't think we're using serde with arm just yet. Can you let us know what errors you've encounter?

@michael2012z
Copy link
Member Author

michael2012z commented May 15, 2020

Hi @andreeaflorescu, some errors appeared when building with "--features with-serde":

> cargo build --features with-serde
   Compiling kvm-bindings v0.2.0 (/home/michael/ws/src/github.com/rust-vmm/kvm-bindings)
error: cannot find derive macro `Deserialize` in this scope
  --> src/arm64/mod.rs:29:43
   |
29 | #[cfg_attr(feature = "with-serde", derive(Deserialize, Serialize))]
   |                                           ^^^^^^^^^^^

error: cannot find derive macro `Serialize` in this scope
  --> src/arm64/mod.rs:29:56
   |
29 | #[cfg_attr(feature = "with-serde", derive(Deserialize, Serialize))]
   |                                                        ^^^^^^^^^

The ch branch of this repo contains some commits for supporting Serde mainly for X86, but one of them includes a little modification for ARM: bc0f648

@andreeaflorescu
Copy link

@michael2012z Can we add this as an issue in kvm-bindings? What rust version are you using? Shouldn't we have a test for this in kvm-bindings?

@michael2012z
Copy link
Member Author

michael2012z commented May 15, 2020

Here comes more clarification about this PR.

What's the problem?

Now Cloud-hypervisor is using ch branch at cloud-hypervisor/kvm-bindings instead of rust-vmm/kvm-bindings. It contains the code for supporting Serde on X86. Some of the code in this branch is from a PR that is on review at rust-vmm/kvm-bindings: rust-vmm#14.

But the code is not ready for AArch64. It doesn't build with "with-serde" feature on AArch64.

This PR is going to fix the build error. In fact, instead of fixing anything, I just removed all ARM code added in the on-review PR just mentioned.

As the code in problem is not in rust-vmm master, but in a PR in-progress. So I can't create a issue to track it.

Why we need to fix it?

Because we are trying to enable Cloud-hypervisor on AArch64. In current stage, we need this branch to build with feature "with-serde" on ARM, after all X86 and AArch64 shares the same Cargo dependency: https://github.com/cloud-hypervisor/cloud-hypervisor/blob/d760010c9e22b60a09beb009ee4ebfb63334ccc7/vmm/Cargo.toml#L23

@michael2012z michael2012z changed the title Revert the modification for supporting Serde on AArch64 Fix the build error with "with-serde" feature in CH branch (on AArch64) May 18, 2020
@michael2012z
Copy link
Member Author

Hi, @aghecenco , could you help give some comments on this fix?

@michael2012z
Copy link
Member Author

@sboeuf , @rbradford , another PR cloud-hypervisor/cloud-hypervisor#1225 encountered the same build error on AArch64. Taking this PR may make things easier.

And we can add some workflows to build on X86 and Arm64 with various feature selections (like those in CI), to verify the upcoming PR's (although there isn't many) against this repo. What do you think?

@Apokleos
Copy link

I have the same error when cargo build on arm64 platform(Linux 4.14.0-115.el7a.0.1.aarch64)with cargo version cargo 1.44.0 (05d080faa 2020-05-06)

In order to pass the compiling, I have to add use serde_derive::{Serialize, Deserialize}; into kvm-bindings-c6b55506be61a061/3a67800/src/arm64/mod.rs.

Maybe it's not a good solution, but it dose work now.

@michael2012z
Copy link
Member Author

I simplified the fix of the build error, it looks more straight-forward.

Now we have to workaround the error in several places:

Fixing the error may make things easier.

@sboeuf
Copy link
Member

sboeuf commented Jul 30, 2020

@michael2012z do you mean with this patch we won't need to do sed -i 's/"with-serde",\ //g' hypervisor/Cargo.toml anymore?

@sboeuf
Copy link
Member

sboeuf commented Jul 30, 2020

/cc @rbradford

@michael2012z
Copy link
Member Author

@sboeuf Yes, that's right. Please merge this.

@michael2012z
Copy link
Member Author

Thank you, @sboeuf , @Apokleos .

I created PR cloud-hypervisor/cloud-hypervisor#1524 to follow up this fix.

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

Successfully merging this pull request may close these issues.

4 participants