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 recipes for just #468

Merged
merged 6 commits into from
Aug 29, 2024
Merged

Add recipes for just #468

merged 6 commits into from
Aug 29, 2024

Conversation

adierkens
Copy link
Member

@adierkens adierkens commented Aug 1, 2024

just has come about as one of the more popular ways of organizing project specific scripts. Given the multi-language and hard-to-find nature of bazel targets, this gives us a clean way of saving and reusing scripts for users.

> just -l                                                                                                                                                                

Available recipes:
    build-all       # Build all targets in the project
    build-core      # Build all core JS/TS files
    build-js        # Build all JS/TS files
    dev-ios         # Generate and open the xcodeproj for Player
    lint-js         # Lint all JS/TS files
    mvn-install     # Install all generated artifacts into the system .m2 repository
    maven-install   # alias for `mvn-install`
    start-docs      # Run a dev server of the main docs page
    start-ios-demo  # Build and run the iOS demo app in a simulator
    start-storybook # Run a dev server of storybook
    test-all        # Test targets in the project
    test-core       # Test all core JS/TS files
    test-js         # Test all JS/TS files

@adierkens adierkens added the internal Changes only affect the internal API label Aug 1, 2024
@adierkens adierkens changed the title Add recipes for [just](https://github.com/casey/just) Add recipes for just Aug 1, 2024
Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.94%. Comparing base (7e8ca9c) to head (f8e3f88).
Report is 155 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #468   +/-   ##
=======================================
  Coverage   91.94%   91.94%           
=======================================
  Files         340      340           
  Lines       26838    26838           
  Branches     1946     1946           
=======================================
+ Hits        24675    24677    +2     
+ Misses       2149     2147    -2     
  Partials       14       14           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

justfile Outdated
@@ -0,0 +1,57 @@
[doc('Build all targets in the project')]
build-all:
bazel build //...
Copy link
Member

Choose a reason for hiding this comment

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

My 2c, we shouldn't have any build //*/... commands in here because:

  1. They'll build a lot of intermediate targets that may not be necessary for checking if the code works
  2. Not all targets are buildable by themselves. Some of them require the context of what they're being built for, i.e. cc_library compilation changes depending on whether it's for JVM or Android. If there is no default set up, likely because is doesn't make sense to build that target by itself, then this would fail.

These scripts should be intent based, "what are you building for" -- testing, publishing, executing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with the general sentiment, but I'm not sure I'd go so far to say we shouldn't have any //... commands here. I don't expect them to be used very often, but for things like testing, we may want to test the full project.

Not all targets are buildable by themselves

I'll admit I'm not as familiar with the standard patterns for cc_library -- but if that truly is the case, then I'd expect we should exclude those targets from the build-all target. Maybe it's more beneficial to convert this into a macro for building all of the sub-systems:

build-core
build-plugins
build-android
build-react
build-ios

which I assume is similar to what we'd want to do for lint-all, test-all, etc

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I'd go so far to say we shouldn't have any //... commands here. I don't expect them to be used very often, but for things like testing

Using ... expansion for testing is fine, but that's because tests explicitly declare how the dependencies should be built. But I'm sticking to avoiding it for build commands -- you probably don't need to build the targets used for publishing locally, at least at the build-all level. It's somewhat misleading to expect all the tooling targets to also be build with a build-all command.

Copy link
Member

Choose a reason for hiding this comment

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

This also isn't limited to just the cc_library targets - any Bazel target may be transitioned upon to change the underlying toolchain. It's called out in the iOS docs for the Swift-based targets:
https://github.com/player-ui/player/tree/main/ios#bazel

bazel test //core/...

[doc('Lint all JS/TS files')]
lint-js:
Copy link
Member

@sugarmanz sugarmanz Aug 1, 2024

Choose a reason for hiding this comment

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

Do we want to have language discrimination for common language things? Practically for performance, maybe, but if I want to lint the project, ideally I'd just lint the whole project.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see any downside to having per language targets in addition to global targets.

Copy link
Member Author

@adierkens adierkens Aug 2, 2024

Choose a reason for hiding this comment

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

I feel like people are more likely to want to lint smaller subsets of the project based on what they're updating rather than the full project all the time -- but to @KetanReddy's point, we can include other targets as needed.

@@ -23,6 +23,9 @@ If the changes are larger (API design, architecture, etc), [opening an issue](ht
* [Signed Commits](https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification). For convenience it is recommended to set git to sign all commits by default as mentioned [here](https://docs.github.com/en/authentication/managing-commit-signature-verification/telling-git-about-your-signing-key)

## Building and Testing Locally (All platforms)

> This project also contains [just](https://github.com/casey/just) recipes for many common commands. They can be listed using `just -l`
Copy link
Member

Choose a reason for hiding this comment

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

Should installing just get added to the setup/contributing guides?

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured we would first see how well we like using just before fully converting the usage guides over. Then we can update the examples commands and stuff to reference it.

@adierkens adierkens added the skip-release Preserve the current version when merged label Aug 5, 2024
@adierkens adierkens merged commit 9d3589e into main Aug 29, 2024
4 of 7 checks passed
@adierkens adierkens deleted the just branch August 29, 2024 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Changes only affect the internal API skip-release Preserve the current version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants