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

Taskfile: Download and set up Meteor.js as part of building the package; Refactor tarball download and extraction into reusable task. #363

Merged
merged 25 commits into from
May 2, 2024

Conversation

junhaoliao
Copy link
Collaborator

@junhaoliao junhaoliao commented Apr 24, 2024

References

  1. Meteor official install script: https://install.meteor.com/
  2. Purpose of .meteor-portable

Description

  1. Taskfile: Add Meteor setup task in Taskfile.yml.
  2. Remove Meteor setup steps from GitHub workflow.
  3. Refactor Taskfile and lint-tasks.yml to extract common download and extract task.
  4. Remove temp files resulted from meteor build before checksum.

Validation performed

  1. Started with a clean Ubuntu 20.04 WSL installation.
  2. Installed Docker Engine following instructions at https://docs.docker.com/engine/install/ubuntu/ .
  3. Ran docker image ghcr.io/y-scope/clp/clp-core-dependencies-x86-ubuntu-focal:main .
  4. Installed Go Task following instructions at https://taskfile.dev/installation/#install-script . Note the "Install Script" approach is taken to avoid installing any Node.js dependencies.
  5. Built the CLP package with command:
    cd <clp-repo>
    task
    
    Observed the build is successful.
  6. Re-ran command task and observed no tasks are getting unnecessarily re-built:
    > task
    task: Task "package-venv" is up to date
    task: Task "core-submodules" is up to date
    task: Task "core" is up to date
    task: Task "clp-package-utils_venv" is up to date
    task: Task "clp-py-utils_venv" is up to date
    task: Task "job-orchestration_venv" is up to date
    task: Task "clp-package-utils" is up to date
    task: Task "clp-py-utils" is up to date
    task: Task "job-orchestration" is up to date
    task: Task "Download & Extract node-v14.21.3-linux-x64.tar.xz" is up to date
    task: Task "Download & Extract meteor-bootstrap-os.linux.x86_64.tar.gz" is up to date
    task: Task "webui-node-modules" is up to date
    task: Task "webui" is up to date
    task: Task "package" is up to date
    
  7. Ran linting config:
# Task `lint:js-check` will also be run, which invokes `linter-nodejs`
task lint:check

And observed success.
11. Deleted <clp-repo>/build/meteor.md5 and ran task. Observed tasks "Download & Extract meteor-bootstrap-os.linux.x86_64.tar.gz" getting rebuilt.

@junhaoliao junhaoliao requested a review from haiqi96 April 24, 2024 01:24
@junhaoliao junhaoliao removed the request for review from haiqi96 April 24, 2024 01:30
@junhaoliao
Copy link
Collaborator Author

Refactoring and validating in progress. Let's put this review on hold.

@junhaoliao junhaoliao changed the title WIP - Taskfile: Add Meteor setup task in Taskfile.yml. Taskfile: Add Meteor setup task in Taskfile.yml. Apr 24, 2024
@junhaoliao junhaoliao requested a review from haiqi96 April 24, 2024 04:19
Taskfile.yml Outdated Show resolved Hide resolved
Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

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

This is neat. Some refactoring comments mostly.

Taskfile.yml Outdated Show resolved Hide resolved
Taskfile.yml Show resolved Hide resolved
Taskfile.yml Outdated Show resolved Hide resolved
Taskfile.yml Outdated Show resolved Hide resolved
Taskfile.yml Outdated Show resolved Hide resolved
Taskfile.yml Outdated Show resolved Hide resolved
Taskfile.yml Outdated Show resolved Hide resolved
Taskfile.yml Outdated Show resolved Hide resolved
Taskfile.yml Show resolved Hide resolved
Taskfile.yml Outdated Show resolved Hide resolved
@@ -182,20 +183,24 @@ tasks:
cmds:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@haiqi96 I noticed we have this platform attribute for task webui. Does it mean the task only runs on x86 platforms? Now that we also set up the arch-specific meteor and nodejs, do we still need the restriction here? (My insights are limited as I'm not too sure how the webui is built currently for your workflow

Copy link
Member

Choose a reason for hiding this comment

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

Does meteor build --architecture now support ARM platform IDs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry for the oversight. It doesn't seem to be the case.
The aarch64 related handling was added into the install script together with the 2.14 -> 2.15 upgrade so I mistakenly thought it was available for download. https://static.meteor.com/packages-bootstrap/2.15/meteor-bootstrap-os.linux.aarch64.tar.gz just doesn't open.

ARM support is expected to be available in meteor 3.0 though.

Taskfile.yml Outdated Show resolved Hide resolved
@@ -206,59 +212,96 @@ tasks:
generates: ["{{.CHECKSUM_FILE}}"]

webui-nodejs:
Copy link
Member

Choose a reason for hiding this comment

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

Not a big deal to leave it since we'll soon clean up the ordering of tasks, but we could move webui-nodejs after webui-node-modules to maintain an alphabetical ordering of these artifact-building internal tasks.

Copy link
Collaborator Author

@junhaoliao junhaoliao May 1, 2024

Choose a reason for hiding this comment

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

Let's leave it to the future PR then. Earlier I also wanted to move task meteor to some upper position but was confused by the current ordering of the non-internal tasks.

Taskfile.yml Outdated Show resolved Hide resolved
kirkrodrigues
kirkrodrigues previously approved these changes May 1, 2024
Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

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

How about:

Taskfile: Download and set up Node.js and Meteor.js as part of building the package. (#363)

@junhaoliao
Copy link
Collaborator Author

Somehow GitHub's git implementation wasn't able to resolve deltas in the moved building.md, and I had to manually trigger a merge offline. Approvers please approve and trigger the merge from your end.

@junhaoliao
Copy link
Collaborator Author

How about:

Taskfile: Download and set up Node.js and Meteor.js as part of building the package. (#363)

Maybe we can give less emphasis on the Node.js setup as webui-nodejs was already there but only got refactored. How about:

Taskfile: Download and set up Meteor.js as part of building the package; Refactor Tarball download and extract into reusable task. (#363)

Feel free to revise the wording and please trigger the merge from your end.

@kirkrodrigues kirkrodrigues changed the title Taskfile: Add Meteor setup task in Taskfile.yml. Taskfile: Download and set up Meteor.js as part of building the package; Refactor tarball download and extraction into reusable task. May 2, 2024
@kirkrodrigues kirkrodrigues merged commit 7fd4082 into y-scope:main May 2, 2024
1 check passed
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