Skip to content

Disable jailer build for Glibc #2102

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

Closed
alindima opened this issue Aug 27, 2020 · 7 comments
Closed

Disable jailer build for Glibc #2102

alindima opened this issue Aug 27, 2020 · 7 comments

Comments

@alindima
Copy link
Contributor

alindima commented Aug 27, 2020

Currently, the jailer can be used only when Firecracker is linked with static musl, not with dynamic glibc.

The fact that the build command successfully builds the jailer with glibc is misleading and outputs jailer binaries that do not work with the corresponding Firecracker binary.

We should make sure that when we compile the source code, we only build the jailer if the specified toolchain is musl.

One way of doing this would be to modify devtool. However, we should first investigate for any options we could add in the Cargo.toml files to restrict compilation based on toolchain.

@krishnakumar4a4
Copy link
Contributor

Hi, after spending few hours on the analysis. Following options can possibly be ruled out:

  • a 'build-only-for-target-abi=musl' like flag in Cargo.toml at crate level is not available.
  • a custom build.rs at project level which can dynamically modify workspace dependencies is not available.
  • Have also checked if we can place a build.rs at the root of jailer crate level that can skip building itself if not matching specified ABI. This is not available too.

Would like to explore more if there are any pointers. Otherwise, devtool modification seems to be the only option.

Thanks,
Krishna Kumar.

@alindima
Copy link
Contributor Author

Hello. Thank you for the interest in this issue and for taking the time to investigate.

In regards to the last point you made, about the build.rs file for jailer, I think there may be an option. According to the docs, if the build script returns an error code, the build process is halted.

We currently have a build script that is run for both the jailer and the firecracker binaries. We could update the script to check the targeted libc abi, before building the jailer. We have two options here: either displaying a warning or exiting with an error code.

Let me discuss this with the team and I will get back to you when we reach a conclusion.

@dianpopa
Copy link
Contributor

Seems that there is no way of specifying workspace members based on platform.
What I see as the most elegant way of doing this is to add a default_members = [src/firecracker] in the root Cargo.toml and modify devtool to only build the jailer only for musl target. (you can use cargo build -p jailer for that).

@acatangiu
Copy link
Contributor

What I see as the most elegant way of doing this is to add a default_members = [src/firecracker] in the root Cargo.toml and modify devtool to only build the jailer only for musl target. (you can use cargo build -p jailer for that).

I like that! devtool does the right thing for both gnu and musl, while manual building can be done as simple as:

# for musl:
cargo build             # <-- builds firecracker
cargo build -p jailer   # <-- builds jailer

# for gnu:
cargo build --target x86_64-unknown-linux-gnu

@alindima
Copy link
Contributor Author

alindima commented Sep 4, 2020

After extensive discussions in the team, we decided to go for the option mentioned above by @dianpopa and @acatangiu.
That is, adding the firecracker binary crate as the only default member of the workspace and modifying devtool to run the two build commands, in order to build both binaries, only for musl (cargo build and cargo build --release).
This would also imply an update in the CHANGELOG.md.

@krishnakumar4a4 are you interested in contributing a fix for this issue? If so, please tell us and get in touch if you need any guidance.

@krishnakumar4a4
Copy link
Contributor

Sure @alindima , would work on it and come up with a PR on this. Will let you know in case I need help. Thanks.

@alindima
Copy link
Contributor Author

Fixed by #2125

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

No branches or pull requests

4 participants