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

feat(ci): add OpenHarmony ci support #1703

Closed
wants to merge 11 commits into from

Conversation

richerfu
Copy link
Contributor

@richerfu richerfu commented Sep 24, 2024

Add minimal OpenHarmony CI testing as requested in #1702 (comment).

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality

@richerfu richerfu changed the title feat(ci): add OpenHarmony ci support DRAFT: feat(ci): add OpenHarmony ci support Sep 24, 2024
@richerfu richerfu changed the title DRAFT: feat(ci): add OpenHarmony ci support feat(ci): add OpenHarmony ci support Sep 25, 2024
@richerfu
Copy link
Contributor Author

The CI of ohos needs to do some extra processing, and I will explain why it is implemented in this way:

  1. First of all, the construction of ohos is not natively supported, we need to do some environment initialization.ohrs is a tool that can help us simplify the use of cargo commands under the ohos build environment.
  2. After the rust version of 1.78.0, ohos is only a tier2 stage target in rust, so it is difficult for us to simply run cargo commands in rust 1.70.0 for ohos, so we will skip the corresponding clippy detection, unless we update them all to versions after 1.78.0.

Could you provide some advice? @MarijnS95

@MarijnS95
Copy link
Member

  1. That is only needed for compilation and linking right? I suggest we only run clippy to make sure things are in a compilable state, which seems to work in a cross-compilation environment as outlined in feat: add openharmony support #1702 (comment).
  2. Hmm that's unforunate. We could try running OHOS clippy on 1.78 though newer linter warnings may show up. It looks like this version-lock may be used to enforce our MSRV though I'm not opposed to warn on and address lints on newer Rust versions (which may be nontrivial when clippy suggests changes that require a higher MSRV).

@richerfu
Copy link
Contributor Author

Yep, so should we upgrade MSRV to 1.78.0? If just run clippy, some steps can be removed.

@richerfu
Copy link
Contributor Author

Evenything seems ok now.

I upgrade rust version to 1.78.0 for CI and keep MSRV for crates. wgl_binding gets a warning in 1.78.0 and fixed it.
image

@MarijnS95

@kchibisov
Copy link
Member

tbf, instead of adding more targets, I'd rather remove cfg, since rwh doesn't need any cfg at all. Like there's nothing wrong in always compiling a bit more code, but you'd know that it's properly tested.

If you pass e.g. Android handle on general linux stuff, nothing wrong will happen, you'll just get an error and that's about it.

@MarijnS95
Copy link
Member

@kchibisov I've previously tried to add cfg guards consistently because they were only put in place "randomly", and found that it was also necessary to disambiguate i.e. the type of a global let binding (IIRC a Vec). In the end I ended up removing most cfg guards that were hiding code which would compile on all platforms. Glad to see/hear that trend continue.


I upgrade rust version to 1.78.0 for CI and keep MSRV for crates. wgl_binding gets a warning in 1.78.0 and fixed it.

Please don't post screenshots but copy-paste logs/code/links, as they are hard to find/reference/quote otherwise.
Maybe we should check why the generator emits unused bindings?

@kchibisov
Copy link
Member

@MarijnS95 we have x11/wayland cfgs for a reason to not build x11/wayland deps, since we use them. The rest doesn't have any system deps, so it all doesn't matter for them.

Like cfg only needed, once wa have something using a system dependency, if no dependency is ever brought, there's no need to cfg anything.

@MarijnS95
Copy link
Member

MarijnS95 commented Oct 3, 2024

@kchibisov I think you are saying the same thing. See the following example, where the cfgs for X11/Wayland are not needed because the code can compile without it anyway as it does not reference any of the deps:

#[cfg(wayland_platform)]
RawDisplayHandle::Wayland(handle)
if extensions.contains("EGL_KHR_platform_wayland") =>
{
(egl::PLATFORM_WAYLAND_KHR, handle.display.as_ptr())
},
#[cfg(x11_platform)]
RawDisplayHandle::Xlib(handle) if extensions.contains("EGL_KHR_platform_x11") => {
attrs.push(egl::PLATFORM_X11_SCREEN_KHR as EGLAttrib);
attrs.push(handle.screen as EGLAttrib);
(
egl::PLATFORM_X11_KHR,
handle.display.map_or(egl::DEFAULT_DISPLAY as *mut _, |d| d.as_ptr()),
)
},

(Hence I removed them in the previously linked commit/PR)

@kchibisov
Copy link
Member

yeah, that's fine then.

@kchibisov
Copy link
Member

Handled in #1707

We just always build the code for harmony now.

@kchibisov kchibisov closed this Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants