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

ci(getting-started): run (but ignore) checks with npm and npx #8250

Merged
merged 7 commits into from Aug 29, 2023

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented Aug 24, 2023

Description

Begin testing the getting started instructions using npm install -g agoric and npx agoric ....

This currently fails because of an incompatibility with @endo/bundle-source@2.5.2's dependency on a patched version of Rollup downloaded from https://github.com/endojs/endo, which causes a problem when NPM autoruns its prepare script.

So, continue-on-error just because those combinations are currently failing.

@kriskowal the next (after 2023-08-23) Endo release will solve the problem when it is used in Agoric SDK.

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

Allows updating our Getting Started instructions with new possibilities for obtaining the Agoric CLI:

  1. current git clone -b community-dev https://github.com/Agoric/agoric-sdk && cd agoric-sdk && yarn install && yarn build && yarn create-agoric-cli /usr/local/bin/agoric
  2. yarn global add agoric@community-dev
  3. npm install -g agoric@community-dev
  4. alias agoric="npx agoric@community-dev"

Testing Considerations

Requires attention to the next Endo merge.

Upgrade Considerations

n/a

@michaelfig michaelfig self-assigned this Aug 24, 2023
@michaelfig michaelfig added agoric-cli package: agoric-cli tooling repo-wide infrastructure force:integration Force integration tests to run on PR labels Aug 24, 2023
@kriskowal
Copy link
Member

We added a mention to check for issues with the next release label during the release process but I see that it seems to have been removed. In any case, I’ll try to remember this change.

@erights
Copy link
Member

erights commented Aug 27, 2023

Requires attention to the next Endo merge.

Hi @michaelfig , could you spin off a minimally different copy of this PR with your #endo-branch: master directive so we can see how compat it already is with endo master, and what the remaining problems are? Thanks.

Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

Approving contingent on the simpler Lerna patch

@@ -29,7 +29,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
cli: [link-cli, local-npm]
cli: [link-cli, local-npm, local-npm/npm, local-npm/npx]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cli: [link-cli, local-npm, local-npm/npm, local-npm/npx]
cli: [link-cli/yarn, registry/yarn, registry/npm, registry/npx]

😄

packages/deployment/package.json Show resolved Hide resolved
@@ -63,7 +63,6 @@
"import-meta-resolve": "^2.2.1"
},
"files": [
"bundles",
Copy link
Member

Choose a reason for hiding this comment

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

@dckc and I have talked about publishing bundles but haven't though through it fully.

(@michaelfig mentioned that including it here adds 20MB, making it too big for repos that have a limit, like verdaccio at 10MB)

patches/@lerna+version+5.6.2.patch Outdated Show resolved Hide resolved
scripts/local-npm.sh Outdated Show resolved Hide resolved
scripts/local-npm.sh Outdated Show resolved Hide resolved
scripts/local-npm.sh Outdated Show resolved Hide resolved
Comment on lines 137 to 138
test -z "${LOCAL_NPM_HOME-}" || export HOME="$LOCAL_NPM_HOME"
test -z "${LOCAL_NPM_DISTTAG-}" || export DISTTAG="$LOCAL_NPM_DISTTAG"
Copy link
Member

Choose a reason for hiding this comment

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

not clear to me why not: HOME = HOME || LOCAL_NPM_HOME.

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 actually want LOCAL_NPM_HOME && (HOME=LOCAL_NPM_HOME) but I can't find a more concise way to write it.

@michaelfig
Copy link
Member Author

could you spin off a minimally different copy of this PR with your #endo-branch: master directive so we can see how compat it already is with endo master, and what the remaining problems are? Thanks.

@mhofman and I are working on getting the nightly scheduled test against master to green, and it will notify us when there is breakage. I'll send you subscription instructions out-of-band, if that's something you're interested in.

@michaelfig michaelfig added the automerge:no-update (expert!) Automatically merge without updates label Aug 29, 2023
Copy link
Member

mhofman commented Aug 29, 2023

@Mergifyio requeue main

Copy link
Member

mhofman commented Aug 29, 2023

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Aug 29, 2023

requeue main

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@mergify
Copy link
Contributor

mergify bot commented Aug 29, 2023

refresh

✅ Pull request refreshed

Copy link
Member

mhofman commented Aug 29, 2023

@Mergifyio queue main

@mergify mergify bot merged commit 3ce0599 into master Aug 29, 2023
78 of 83 checks passed
@mergify
Copy link
Contributor

mergify bot commented Aug 29, 2023

queue main

✅ The pull request has been merged automatically

The pull request has been merged automatically at 3ce0599

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agoric-cli package: agoric-cli automerge:no-update (expert!) Automatically merge without updates force:integration Force integration tests to run on PR tooling repo-wide infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants