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.

Sign up for GitHub

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

Jump to bottom

ci: Specify to build winit package when invoking cargo-apk #2561

Merged
merged 3 commits into from
Nov 24, 2022
Merged

ci: Specify to build winit package when invoking cargo-apk #2561

merged 3 commits into from
Nov 24, 2022

Conversation

MarijnS95
Copy link
Member

@MarijnS95 MarijnS95 commented Nov 21, 2022 •
edited

Since rust-mobile/cargo-subcommand#23 cargo-apk now strictly searches for workspaces first before committing to finding the right package within said workspace, and bails when no package was selected since we don't support selecting (building, packaging, running) >1 target currently.

Perhaps it's a bit hash to enforce this on free-form cargo apk -- invocations, but it is what it is.

@MarijnS95 MarijnS95 mentioned this pull request Nov 21, 2022
Merged
@madsmtm madsmtm added DS - android S - maintenance Repaying technical debt labels Nov 22, 2022
@MarijnS95
Copy link
Member Author

I'll close this soon, now that original behaviour has been restored while using clap in rust-mobile/ndk#363 🎉; winit doesn't need to do anything to upgrade to the latest cargo-apk.

However, this PR does contain some nice cleanups... 😅

@MarijnS95
ci: Don't use $CMD for Android doc building
4b71016 
Since migrating `cargo-apk` to `clap` [it is now annoying] to pass
unknown arguments to an underlying `cargo` command (like `cargo doc`):
fortunately generating docs doesn't need to go through `cargo apk` to
set up cross-compiler/linker environment variables at all.

[it is now annoying]: rust-mobile/ndk#363
@MarijnS95
ci: Simplify
58bf5b9
@MarijnS95
ci: Explicitly build just the winit package on Android
c3f4868 
Since rust-mobile/cargo-subcommand#23 `cargo-apk`
now strictly searches for workspaces first before committing to finding
the right package _within said workspace_, and bails when no package was
selected since we don't support selecting (building, packaging, running)
>1 target currently.

Perhaps it's a bit hash to enforce this on free-form `cargo apk --`
invocations, but it is what it is.
@MarijnS95
Copy link
Member Author

Error: Multiple packages are not yet supported (ie. in a [workspace]). Use -p to select a specific package.

Ah, uhm, looks like the latest cargo-apk push broke the CI again, even though I just tested it with one PR. Looks like another one silently broke it: rust-mobile/ndk#360 + rust-mobile/cargo-subcommand#23.

This probably wasn't an issue before because we didn't go looking for workspaces if we found our [package] right in the current/root Cargo.toml, but I changed that in rust-mobile/cargo-subcommand#23 to go and find a workspace first.

Might be simple the solve with options: -p winit though, no need to test/build the run-wasm crate on Android.

madsmtm
madsmtm approved these changes Nov 23, 2022
View reviewed changes
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

I'm fine with the options: -p winit fix

@MarijnS95
Copy link
Member Author

Looks like the CI is all green now!

Fwiw it's not strictly a "fix" but more so explicitly specifying what we want cargo apk to do; OTOH cargo has defined ways in which it selects the package to build when not using --all/--workspace, but I vaguely recall writing that that logic was vastly complicated and error prone (based on $PWD, cmdline flags and default configs in [workspace]).

@MarijnS95
Copy link
Member Author

Restarting one job, https://github.com/rust-windowing/winit/actions/runs/3533140877/jobs/5928396199 just got stuck for 15 minutes...

@MarijnS95 MarijnS95 marked this pull request as ready for review November 23, 2022 15:27
@MarijnS95 MarijnS95 changed the title ci: Update to clap-backed cargo-apk ci: Specify to build winit package when invoking cargo-apk Nov 23, 2022
@MarijnS95 MarijnS95 merged commit ba4bf03 into rust-windowing:master Nov 24, 2022
@MarijnS95 MarijnS95 deleted the ci-android branch November 24, 2022 20:49
@MarijnS95
Copy link
Member Author

I keep forgetting that I have merge rights here 😬 - merging this in now so that stuff merged to master no longer results in commits with failed CI...

@MarijnS95
Copy link
Member Author

MarijnS95 commented Nov 26, 2022 •
edited

Fwiw I'm trying to back out of this change by reimplementing default package selection:

rust-mobile/cargo-subcommand#25
https://github.com/rust-windowing/android-ndk-rs/issues/371

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@madsmtm madsmtm madsmtm approved these changes

Assignees
No one assigned
Labels
DS - android S - maintenance Repaying technical debt
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@MarijnS95 @madsmtm