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

build: implement build android workflow #51666

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

MeowShe
Copy link
Contributor

@MeowShe MeowShe commented Feb 5, 2024

Experimental activation of CI Workflow for building on the Android platform.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/actions

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Feb 5, 2024
@MeowShe MeowShe changed the title ci: build-android.yaml ci: implement build android orkflow Feb 5, 2024
@MeowShe MeowShe changed the title ci: implement build android orkflow ci: implement build android workflow Feb 5, 2024
@MeowShe MeowShe changed the title ci: implement build android workflow build: implement build android workflow Feb 5, 2024
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

RSLGTM

.github/workflows/build-android.yaml Outdated Show resolved Hide resolved
tniessen
tniessen previously approved these changes Feb 12, 2024
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

All other files use the extension .yml as far as I can tell. I'd suggest renaming to build-androi.yml for consistency.

.github/workflows/build-android.yaml Outdated Show resolved Hide resolved
@tniessen tniessen dismissed their stale review February 12, 2024 21:25

Didn't mean to hit approve

@MeowShe
Copy link
Contributor Author

MeowShe commented Feb 13, 2024

All other files use the extension .yml as far as I can tell. I'd suggest renaming to build-androi.yml for consistency.

You're right, it's a mistake.

@targos
Copy link
Member

targos commented Feb 13, 2024

  • Is there any way to try and run it before merging?
  • Is it going to block commit queue when it fails?

@MeowShe
Copy link
Contributor Author

MeowShe commented Feb 13, 2024

  • Is there any way to try and run it before merging?

  • Is it going to block commit queue when it fails?

  1. We are waiting for another PR merge regarding Android build fixes. (build: remove librt library dependency for Android compatibility #51632)

  2. This is an experimental feat., it shouldn't block commit queue even if fails.

@targos
Copy link
Member

targos commented Feb 13, 2024

This is an experimental feat., it shouldn't block commit queue even if fails.

I know it shouldn't, but I'm afraid it will (it runs on pull_request event).

@tniessen
Copy link
Member

Is it going to block commit queue when it fails?

GitHub unfortunately still does not support allowed failures (see actions/runner#2347), but perhaps one could use continue-on-error somehow?

@targos
Copy link
Member

targos commented Feb 22, 2024

We can use continue-on-error. It will make the run always green.
I'm still sceptical about having feedback on all pull requests for a very experimental target.
A new actions run will use a worker from the pool and add a pending check to the pull request while it runs.

@MeowShe
Copy link
Contributor Author

MeowShe commented Feb 23, 2024

@tniessen @targos continue-on-error has been used already(dea50f6).

@MeowShe
Copy link
Contributor Author

MeowShe commented Feb 25, 2024

I'm still sceptical about having feedback on all pull requests for a very experimental target.

A new actions run will use a worker from the pool and add a pending check to the pull request while it runs.

The main purpose of this pull request is to advance the official support of Node.js for the Android platform. Once the goal is achieved and it becomes stable, this check should become mandatory.

@bnb
Copy link
Contributor

bnb commented Feb 26, 2024

I'm still sceptical about having feedback on all pull requests for a very experimental target.

Perhaps in the short term having it be a path-based Action? Would allow a smaller number of PRs that have it as a check as a way to warm up to having it on all PRs.

@MeowShe
Copy link
Contributor Author

MeowShe commented Feb 27, 2024

Maybe label is a better idea? We label PRs when if it fit to run Android build test.

@MeowShe
Copy link
Contributor Author

MeowShe commented Feb 27, 2024

path based Action? Would allow a smaller number of PRs that have it as a check as a way to warm up to having it on all PRs.

I guess there doesn't seem to be a clear definition of paths, we don't know what paths are available to run Android build tests in particular.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants