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

build: Refactor Taskfile.yml #3445

Merged
merged 10 commits into from
Sep 4, 2023
Merged

build: Refactor Taskfile.yml #3445

Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
215d72b
Refactor Taskfile.yml to provide these commands that build and serve …
richb-hanover Aug 30, 2023
af3de80
Merge branch 'main' into npm-cache
richb-hanover Aug 31, 2023
21bd119
`install-playground-dependencies` now checks crates/ & bindings/ for …
richb-hanover Sep 3, 2023
829772b
Merge commit 'd22a1745ba4a0f6464966780eedaeed615631a7f' into npm-cache
richb-hanover Sep 3, 2023
f1d6741
Merge branch 'npm-cache' of https://github.com/richb-hanover/prql int…
richb-hanover Sep 3, 2023
2e7acd7
Add back check-brew-dependencies that were missed in prior merge;
richb-hanover Sep 3, 2023
2567573
`task test-lint` finds silly problems
richb-hanover Sep 3, 2023
265a565
Update Taskfile.yml
richb-hanover Sep 4, 2023
07c8691
Editorial tweaks
richb-hanover Sep 4, 2023
530714d
Use "task:" when calling internal tasks (it seemed to work without th…
richb-hanover Sep 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 46 additions & 35 deletions Taskfile.yml
View file
Original file line number Diff line number Diff line change
@@ -201,6 +201,7 @@ tasks:
- brew install {{.brew_dependencies | trim | splitLines | join " " }}

install-npm-dependencies:
# Use npm to install important tools
cmds:
- npm install --location=global prettier prettier-plugin-go-template
- cmd:
@@ -213,7 +214,6 @@ tasks:

build-all:
desc: Build everything.

summary: |
Build everything.

@@ -284,18 +284,16 @@ tasks:
- cargo insta test --accept -p prql-compiler --lib -- --quiet

build-web:
desc: Build the website, including the book & playground.
desc:
Build the web products for distribution - Website, Book, and Playground.
dir: web
cmds:
- cd website && hugo --minify
- cd book && mdbook build
- mkdir -p website/public
# Copy the book into the website path, using rsync so it only copies new files
- rsync -ai --checksum --delete book/book/ website/public/book/
# Must set `install-links=false` in the playground's `npm install` to
# install prql-js as the regular dependency, instead of creating a
# symlink. Refer to https://github.com/PRQL/prql/pull/1296.
- cd playground && npm install
- cd playground && task update-playground-dependencies
- cd playground && npm run build
# We place the playground app in a nested path, because we want to use
# prql-lang.org/playground with an iframe containing the playground.
@@ -324,56 +322,50 @@ tasks:
- pre-commit run -a

run-website:
desc: Build & serve the static website.
desc: Build & serve the static website for interactive development.
dir: web/website
cmds:
- hugo server

run-web:
desc: Build & serve the website & playground.
summary:
Note that this only live-reloads the static website; and requires
rerunning to pick up playground & book changes.
dir: web/website
cmds:
- task: build-web
# Then start web server with rendering to disk, so it picks up the playground files.
- hugo server --renderToDisk

run-book:
desc: Build & serve the book.
desc: Build & serve the book for interactive development.
dir: web/book
cmds:
- mdbook serve --port=3000

run-playground:
desc: Build & serve the playground.
dir: web/playground
cmds:
- npm install
- npm start

run-playground-cached:
desc: Build & serve the playground, without rebuilding rust code.
desc: Build & serve the playground for interactive development.
dir: web/playground
cmds:
- task: install-playground-npm-dependencies
- task update-playground-dependencies
- npm start

install-playground-npm-dependencies:
# Look to see if npm dependencies need to be updated
# Use task's sources/generates to see if checksums of
# node_modules directory have changed since package.json was updated

desc: Check to see if package.json has changed since we ran npm install
update-playground-dependencies:
# Check if npm dependencies for the playground need to be updated
# Use task's sources/generates to see if package.json,
# or anything in crates or bindings was updated after the
# node_modules was rebuilt
desc: Install dep's if something important changed recently
dir: web/playground
cmds:
- npm install
- task install-playground-dependencies-all
sources:
- package.json
- ../../crates/**/*.rs
- ../../bindings/**/*.rs
generates:
- node_modules/*

install-playground-dependencies-all:
Copy link
Member

Choose a reason for hiding this comment

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

Can we just combine this with the previous? Passing --force can override the sources check if we ever want to force an install

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'd like to leave it like this. I factored it out when --install-links=false was necessary (only wanted to have that magic invocation in one place).

Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between calling one vs the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand the thrust of this question: "combine with the previous"? Which two are you suggesting could be combined?

Here's my understanding of the situation: In the original Taskfile.yaml there were several invocations of npm install --install-links=false which I factored into a single install-playground-dependencies-all task so the magic --install-links... wouldn't be screwed up in one of the multiple places. I also described that option in the comments (again, in one place). Additionally, I changed all the places that called npm install... to use the install-playground-dependencies task so they could take advantage of caching prior results.

Now that the --install-links=false is no longer required, we certainly could get rid of the (single) call to install-playground-dependencies-all and replace it with a simple npm install

But I'll also point out the "documentation advantage" of having a named task that unconditionally runs the proper npm install command in the case that the logic behind the install-playground-dependences gets screwed up. Instead of needing to remember where the --force option might be used, people can just task install-playground-dependencies-all to un-screw it.

I hope that addresses your question

Copy link
Member

@max-sixty max-sixty Sep 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What's the difference between install-playground-dependencies-all and install-playground-dependencies? I think one just checks the sources. So can we combine them?

Copy link
Member

Choose a reason for hiding this comment

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

Can we combine these though? I don't see why we have a task that seems to just wrap another task?

# `npm install --install-links=false` no longer necessary
# Formerly it was used to install prql-js
# as the regular dependency, instead of creating a
# symlink. Refer to https://github.com/PRQL/prql/pull/1296.
desc: Force npm install for all playground packages
dir: web/playground
cmds:
- npm install

build-php:
- cargo build -p prql-lib --release
- mkdir -p bindings/prql-php/lib/
@@ -417,3 +409,22 @@ tasks:
# - curl -fsSL https://deb.nodesource.com/setup_16.x | bash -
# - apt install -y nodejs
# - cd /app/playground/ ; npm install

# These tasks have been factored out in favor of the remaining tasks
# run-web:
# desc: Build & serve the website & playground.
# summary:
# Note that this only live-reloads the static website; and requires
# rerunning to pick up playground & book changes.
# dir: web/website
# cmds:
# - task: build-web
# # Then start web server with rendering to disk, so it picks up the playground files.
# - hugo server --renderToDisk

# run-playground-cached:
# desc: Build & serve the playground, without rebuilding rust code.
# dir: web/playground
# cmds:
# - task: install-playground-dependencies
# - npm start