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

Add custom platform configuration #46

Closed

Conversation

AlixANNERAUD
Copy link
Contributor

Hello,
This PR adds support for custom platforms by setting the environment variables WAMR_SHARED_PLATFORM_CONFIG and WAMR_BUILD_PLATFORM, which propagate to the WAMR build scripts.

@lum1n0us
Copy link
Collaborator

May I ask how to use this feature?

@AlixANNERAUD
Copy link
Contributor Author

As described in the porting guide, the WAMR_BUILD_PLATFORM (which contains the platform name) and/or WAMR_SHARED_PLATFORM_CONFIG (which is the path to the custom platform CMake file, later translated to SHARED_PLATFORM_CONFIG define) can be specified. This allows the crate user to specify a custom platform and potentially a private custom platform API. Additionally, the ESP-IDF build system can be enabled or disabled manually using the esp-idf feature. This is necessary because the ESP-IDF build procedure differs from the regular one (build internally by esp-idf-sys), which could disrupt the use of a custom platform.

@lum1n0us
Copy link
Collaborator

lum1n0us commented Jul 24, 2024

Does it mean the compilation command will be something like CARGO_CFG_TARGET_OS=espidf cargo build cargo build --target=espidf?

&& (env::var("WAMR_BUILD_PLATFORM").is_ok()
|| env::var("WAMR_SHARED_PLATFORM_CONFIG").is_ok())
{
panic!("ESP-IDF build cannot use custom platform build (WAMR_BUILD_PLATFORM) or shared platform config (WAMR_SHARED_PLATFORM_CONFIG)");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused here. I though ESP-IDF building should use stuff in product-mini/platforms/esp-idf/ in wamr repo. But the panic! seems suggest another way?

Copy link
Contributor Author

@AlixANNERAUD AlixANNERAUD Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is handled by the esp-idf-sys crate (refer to Cargo.toml). Since the WAMR esp-idf build script is designed to build WAMR as an esp-idf component, we cannot use the regular build.rs due to missing libc / esp-idf dependencies.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, the new code suggests the build.rs should quit as soon as possible when realizing it is for an esp-idf build.

If so, is_espidf means cargo --target=espidf and host platform is espidf.

Then we might want to check if the value env::var("WAMR_BUILD_PLATFORM").unwrap() == "esp-idf" instead of if WAMR_BULD_PLATFORM is defined only. I guess it could be something else.

In order to avoid crossing compilation, might like

if is_espidf || env::var("WAMR_BUILD_PLATFORM").unwrap() == "esp-idf"

SHARED_PLATFORM_CONFIG is assumed to match WAMR_BUILD_PLATFORM. So I guess we can ignore it.

Please correct me, if I am wrong.

Copy link
Contributor Author

@AlixANNERAUD AlixANNERAUD Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build script should exit immediately since we still need to generate bindings.rs themselves (which are the same for every platform), which is performed at the end of the build script. However, the underneath linked functions are different and depend on the platform.

When building for ESP-IDF, the host platform is Linux, macOS, or Windows, but the target is espidf (e.g., xtensa-esp32-espidf, riscv-espc3-espidf etc.).

WAMR_BUILD_PLATFORM and SHARED_PLATFORM_CONFIG should not be defined when building for ESP-IDF since it doesn't make sense to declare them; everything is handled by esp-idf-sys.

For more clarity, here are the different possible build scenarios:

Target Feature Env Compile?
esp-idf esp-idf None Yes
esp-idf None WAMR_BUILD_PLATFORM / SHARED_PLATFORM_CONFIG No: WAMR provided build script is intended to be build as an esp-idf component
others None None Yes
others None WAMR_BUILD_PLATFORM / SHARED_PLATFORM_CONFIG Yes
others esp-idf None No: doesn’t make any sense

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build script should exit immediately since we still need to generate

should -> should not?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, when the target is esp-idf, we can ignore WAMR_BUILD_PLATFORM and SHARE_PPLATFORM_CONFIG. Because L31~L64 is protected by if !is_espidf condition. So we can skip cmake compilation and goto bindings generation.

dst.define("SHARED_PLATFORM_CONFIG", &platform_config);
println!("cargo:rerun-if-changed={}", platform_config);
}

Copy link
Collaborator

@lum1n0us lum1n0us Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not for esp-idf. right? It is for other new-os.

So we are using env::var to operate cmake compilation options. Good to learn.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like macos, windows, android, those are OSes can be represented by CARGO_CFG_TARGET_OS(https://doc.rust-lang.org/reference/conditional-compilation.html#target_os), and those platform configuration files (shared_platform.cmake) are listed in WAMR repo(core/shared/platform/XXX/. No need to create).

Therefore, I am thinking that maybe we can set both WAMR_BUILD_PLATFORM and WAMR_SHARE_PLATFORM_CONIG for those OSes.

For example, create a pre-defined dict which keys are possible values(and WAMR supported) of target_os and values are paths for correspond shared_platform.cmake. And firstly use user defined env variables(allow overwrite). If not set, use pre-defined dict.

After above, I guess users can use wamr-rust-sdk in other OSes.

@AlixANNERAUD
Copy link
Contributor Author

Any updates ?

@lum1n0us
Copy link
Collaborator

lum1n0us commented Sep 4, 2024

IIUC, when the target is esp-idf, we can ignore WAMR_BUILD_PLATFORM and SHARE_PPLATFORM_CONFIG.

Is that correct?

So, current protection is enough.

  let no_espidf = env::var("CARGO_CFG_TARGET_OS").unwrap() != "espidf";
  // because the ESP-IDF build procedure differs from the regular one (build internally by esp-idf-sys),
  if no_espidf {
    // ...   
  }

  // bindgen ...

If so, all we need is to involve WAMR_BUILD_PLATFORM and SHARE_PPLATFORM_CONFIG. I can create a new PR to implement it.

@AlixANNERAUD
Copy link
Contributor Author

AlixANNERAUD commented Sep 4, 2024

IIUC, when the target is esp-idf, we can ignore WAMR_BUILD_PLATFORM and SHARE_PPLATFORM_CONFIG.

Is that correct?

So, current protection is enough.

  let no_espidf = env::var("CARGO_CFG_TARGET_OS").unwrap() != "espidf";
  // because the ESP-IDF build procedure differs from the regular one (build internally by esp-idf-sys),
  if no_espidf {
    // ...   
  }

  // bindgen ...

If so, all we need is to involve. I can create a new PR to implement it.

The current protection is sufficient on its own; however, we should inform or warn the user that there may be an issue when setting WAMR_BUILD_PLATFORM and SHARE_PLATFORM_CONFIG with ESP-IDF to avoid any confusion.

That said, I'm not sure why you want to create another PR, since everything related to WAMR_BUILD_PLATFORM and SHARE_PLATFORM_CONFIG is already included here.

@lum1n0us
Copy link
Collaborator

lum1n0us commented Sep 4, 2024

The current protection is sufficient on its own; however, we should inform or warn the user that there may be an issue when setting WAMR_BUILD_PLATFORM and SHARE_PLATFORM_CONFIG with ESP-IDF to avoid any confusion.

In this case, you are right.

That said, I'm not sure why you want to create another PR, since everything related to WAMR_BUILD_PLATFORM and SHARE_PLATFORM_CONFIG is already included here.

Just in case if u don't want to spend too much time on it since it's been a long time and still need some work.

@AlixANNERAUD
Copy link
Contributor Author

The current protection is sufficient on its own; however, we should inform or warn the user that there may be an issue when setting WAMR_BUILD_PLATFORM and SHARE_PLATFORM_CONFIG with ESP-IDF to avoid any confusion.

In this case, you are right.

That said, I'm not sure why you want to create another PR, since everything related to WAMR_BUILD_PLATFORM and SHARE_PLATFORM_CONFIG is already included here.

Just in case if u don't want to spend too much time on it since it's been a long time and still need some work.

What do you mean when you say it still needs some work?

@lum1n0us
Copy link
Collaborator

lum1n0us commented Sep 9, 2024

rebase to resolve conflict

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.

2 participants