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

Speed up cli features GitHub Actions workflow #833

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jeffcharles
Copy link
Collaborator

Description of the change

Uses the same feature configuration for building the default Javy plugin and Javy CLI when building the test plugin.

The really fast way to do this might be to have some way to skip the test plugin integration tests for this workflow.

Another thought that comes to mind is we might be busting the cargo target cache unintentionally since we're always emitting a build with the feature flag enabled as the last build step that runs. We could configure the ci-shared-setup to use a slightly different cache key for the Cargo target cache for this workflow.

Why am I making this change?

This workflow runs quite a bit slower because we have to compile the plugin and the CLI twice because they have different features enabled. Using the same features means it should be able to re-use the build artifacts across steps.

Checklist

  • I've updated the relevant CHANGELOG files if necessary. Changes to javy-cli and javy-plugin do not require updating CHANGELOG files.
  • I've updated the relevant crate versions if necessary. Versioning policy for library crates
  • I've updated documentation including crate documentation if necessary.

Copy link
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

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

Do you have a sense of the slowdown?

If there's a way that we can future-proof the approach, that would be my preference I think: with the current approach we'd probably need to be very careful to ensure that we always update this file every time cargo features change, which will likely end-up going out of sync.

Another thought:

IIRC this workflow only exists to test the output of the experimental event loop. The event loop was marked as experimental by nature of the previous bindings as they didn't offer a great way to model async tasks, that is not the case with rquickjs, I wonder if we should make the event loop an on-or-off by default runtime configuration instead of a cargo feature, which can then be controlled with a -J option. I don't think that the code added by the dropping feature will be substantial, since there's no way to turn it off completely in rquickjs anyway, our cargo feature simply errors out, instead of waiting for completion of the tasks.

@jeffcharles
Copy link
Collaborator Author

The check takes 25 minutes to run. Part of the reason for that is it has to rebuild everything without a cache.

And yeah, it looks like this just tests the experimental event loop. And yeah, we could use a -J flag instead.

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