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

Refactor tasks to not fork, and always run in-process of the main execution pipeline #1613

Merged
merged 31 commits into from
Dec 13, 2022
Merged

Refactor tasks to not fork, and always run in-process of the main execution pipeline #1613

Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
24e157a
Check that Deno v1.23.x is used when building Taqueria
Dec 7, 2022
bc977f5
Temporary commit
Dec 7, 2022
6d4c9b0
Added means of registering internal tasks, and to process those tasks…
Dec 8, 2022
81c3a8f
Segmented global and internal tasks
Dec 8, 2022
2cc3bb8
Ensure that internal tasks are invoked
Dec 8, 2022
eddbde7
Fixed memory leak in test/e2e/utils
Dec 8, 2022
ebe670c
Fix broken test due to help output changing
Dec 8, 2022
ded3e1e
Fixed failing test spec due to help output changes
Dec 8, 2022
ca98a5c
Re-add afterAll hook to cleanup
Dec 8, 2022
ecd86da
Fix broken tests due to help output
Dec 8, 2022
e7b9aac
Fixed help output for contract types test spec
Dec 8, 2022
6412863
Committing work thus far
Dec 9, 2022
60b72ed
Accomodate aliases when determining whether a task is runninng
Dec 9, 2022
4dfd48f
Fixed problem with taq not outputting in non-taquified directory
Dec 9, 2022
1736d8d
Merged main into refactor-internal-tasks
Dec 9, 2022
38e3a76
Skip pinata help output tests
Dec 9, 2022
897629a
Skipping some tests due to help output changing
Dec 9, 2022
7f15da8
Assure that we can identify whether tasks are being run which have sp…
Dec 9, 2022
da066d3
Fixed issue with parsedArgs._ being modified after parseArgs is run
Dec 9, 2022
f704bbf
Ensure that detection for whether a given task is running is working
Dec 10, 2022
c1d4a33
Merge branch 'main' into refactor-internal-tasks
Dec 10, 2022
aca2f32
Skip some tests that fail due to change in help output
Dec 10, 2022
d36d379
Add missing aliases to some registered tasks
Dec 10, 2022
4c10da2
Skipped more tests due to changes in help text output
Dec 10, 2022
faf30f1
Fixed issues with failing tests in metadata test suite due to legacy …
Dec 10, 2022
79f3556
Fixed broken ligo test due to skipping a previous test
Dec 10, 2022
19f6a91
Removed debugger statements
Dec 10, 2022
94408ce
Fixed typo
Dec 10, 2022
10caa14
Added link to issue for todo item
Dec 10, 2022
752ea15
Addressed PR comments
Dec 12, 2022
b405eb0
Merge branch 'prerelease-0.25.0' into refactor-internal-tasks
hu3man Dec 13, 2022
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
28 changes: 16 additions & 12 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ concurrency:
group: ${{ github.ref_name || github.ref }}
cancel-in-progress: true

env:
DENO_VERSION: '1.23.1'

jobs:
# When we use reusable we lose the ability to to filter out the workflow for changes to certain paths.
# This job creates boolean outputs based on path filters. These outputs can then be used as job conditions
Expand Down Expand Up @@ -66,12 +69,6 @@ jobs:
os_short: ubuntu
deno_target: "x86_64-unknown-linux-gnu"
taqueria_bin: "taq"
continue-on-error: false
- os: windows-latest
os_short: windows
deno_target: "x86_64-pc-windows-msvc"
taqueria_bin: "taq.exe"
continue-on-error: true
- os: macOS-latest
os_short: macos
deno_target: "x86_64-apple-darwin"
Expand All @@ -80,7 +77,6 @@ jobs:

outputs:
public-url-ubuntu: ${{ steps.public-url.outputs.ubuntu }}
public-url-windows: ${{ steps.public-url.outputs.windows }}
public-url-macos: ${{ steps.public-url.outputs.macos }}

env:
Expand All @@ -93,7 +89,7 @@ jobs:
- name: Setup Deno
uses: denoland/setup-deno@v1
with:
deno-version: "1.23.4"
deno-version: "${{ env.DENO_VERSION }}"

# - name: Delete node-gyp cache
# id: delete-node-gyp-cache
Expand Down Expand Up @@ -215,7 +211,7 @@ jobs:

- uses: denoland/setup-deno@v1
with:
deno-version: "1.23.4"
deno-version: "${{ env.DENO_VERSION }}"

- name: Add working dir to PATH
run: echo "$(pwd)" >> $GITHUB_PATH
Expand Down Expand Up @@ -311,15 +307,25 @@ jobs:
run: |
echo ${NODE_AUTH_TOKEN} >> ~/.npmrc
npm publish --workspaces --tag 'latest' --access public --registry $NPM_REGISTRY


# Workaround for exposing env context to reusable workflows
# TODO: Refactor once https://github.com/orgs/community/discussions/26671 is resolved
expose-env-vars:
runs-on: ubuntu-latest
outputs:
deno_version: ${{ env.DENO_VERSION }}
steps:
- run: echo "Exposing env.DENO_VERSION for reusable workflow"
vscode-extension-workflow:
needs:
- workflow-setup
- publish-npm-packages
- expose-env-vars
if: ${{ !github.event.pull_request.head.repo.fork }}
uses: ./.github/workflows/vscode-extension.yml
with:
prerelease: ${{ needs.workflow-setup.outputs.prerelease }}
deno-version: ${{ needs.vscode-extension.outputs.deno_version }}
secrets:
GCP_PROJECT: ${{ secrets.GCP_PROJECT }}
GCP_SERVICE_ACCOUNT: ${{ secrets.GCP_SERVICE_ACCOUNT }}
Expand Down Expand Up @@ -382,7 +388,6 @@ jobs:
<p>
<a href="${{ needs.build-binaries.outputs.public-url-ubuntu }}">Linux</a><br>
<a href="${{ needs.build-binaries.outputs.public-url-macos }}">MacOS</a><br>
<a href="${{ needs.build-binaries.outputs.public-url-windows }}">Windows</a>
</p>
</td>
</tr>
Expand Down Expand Up @@ -442,7 +447,6 @@ jobs:
run: |
mkdir release && cd release
wget ${{ needs.build-binaries.outputs.public-url-ubuntu }} -O taq-linux
wget ${{ needs.build-binaries.outputs.public-url-windows }} -O taq-windows.exe
wget ${{ needs.build-binaries.outputs.public-url-macos }} -O taq-macos
wget ${{ needs.vscode-extension-workflow.outputs.vsixGcsUrl }}

Expand Down
5 changes: 4 additions & 1 deletion .github/workflows/vscode-extension.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ on:
prerelease:
required: false
type: string
deno-version:
required: true
type: string
secrets:
GCP_PROJECT:
required: true
Expand Down Expand Up @@ -52,7 +55,7 @@ jobs:
- name: Install Deno
uses: denoland/setup-deno@v1
with:
deno-version: "1.23.4"
deno-version: "${{ inputs.deno-version }}"

- name: Attempt to rebuild
run: npm rebuild | true
Expand Down
13 changes: 9 additions & 4 deletions analytics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,15 @@ export const inject = (deps: UsageAnalyticsDeps) => {
return taqResolve('');
}
}),
mapRej(() =>
option === OPT_IN
? 'The command "taq opt-in" is ignored as this might be the first time running Taqueria...'
: 'The command "taq opt-out" is ignored as this might be the first time running Taqueria...'
mapRej(previous =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need it? I thought this would not be needed after the internal task refactor

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure to be honest. If not, I assume we'll address that in a different ticket/PR.
The acceptance criteria for this particular issue/PR was:

Refactor remaining internal tasks so that the execution flow matches that of plugin-provided tasks

As that criteria is a little vague, I interpreted it to be:

Refactor tasks to not fork but run in the main execution pipeline

The motive for that , as far as I was aware, was to resolve a) memory leaks; b) concurrency issues; c) make the code easier to reason about; d) ability to ask for --help for built-in tasks

I had thought that the only thing holding back GA tracking was issue B above.

--
I modified the following code strictly for type correctness, as this section of code was returning Future<string, unknown> instead of Future<TaqError, unknown>:

Before:

mapRej(() =>
  option === OPT_IN
	  ? 'The command "taq opt-in" is ignored as this might be the first time running Taqueria...'
	  : 'The command "taq opt-out" is ignored as this might be the first time running Taqueria...'
)

After:

mapRej(previous => TaqError.create({
	kind: 'E_OPT_IN_WARNING',
	msg: option === OPT_IN
		? 'The command "taq opt-in" is ignored as this might be the first time running Taqueria...'
		: 'The command "taq opt-out" is ignored as this might be the first time running Taqueria...',
	previous,
	context: option,
})

If the above code isn't needed anymore, great - but I'd probably prefer making that adjustment in a different PR, as I cannot recall the reason why Jimmy added this code, and too, I think that's outside of the scope I had in mind in this PR. Would you be fine if we created an issue for this instead, Ali? I would do so, but given that you recognized this, I assume you might have more context/recollection about why this code was here to start with.

TaqError.create({
kind: 'E_OPT_IN_WARNING',
msg: option === OPT_IN
? 'The command "taq opt-in" is ignored as this might be the first time running Taqueria...'
: 'The command "taq opt-out" is ignored as this might be the first time running Taqueria...',
previous,
context: option,
})
),
);

Expand Down
13 changes: 13 additions & 0 deletions bin/build-all.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,19 @@ if [ "$0" == "./bin/build-all.sh" ] && [ -f index.ts ]; then
echo "❌ Docker is not installed, not running, or the user needs permission."
exit $errorCode
fi
if [ -n `which deno || ""` ]; then
deno --version | grep 1.23 >/dev/null
errorCode=$?
if [ $errorCode -ne 0 ]; then
echo "❌ Deno is installed, but not using v1.23.x. Please use Deno v1.23.x."
exit $errorCode
else
echo "✅ Deno is installed, and running v1.23.x"
fi
else
echo "❌ Deno is not installed."
exit -2
fi
echo "** Dependency checks passed"

set -e # exiting on error
Expand Down
7 changes: 7 additions & 0 deletions bin/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ if [ "$0" == "./bin/build.sh" -a -f index.ts ]; then
echo "Please install deno before attempting to build."
echo "Run: curl -fsSL https://deno.land/install.sh | sh"
else
deno --version | grep 1.23 >/dev/null
errorCode=$?
if [ $errorCode -ne 0 ]; then
echo "Deno is installed, but not using v1.23.x."
exit $errorCode
fi

if [ -z "$DENO_TARGET" ]; then
TARGET_ARG=""
else
Expand Down
Loading