-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one possible CI optimization. Otherwise lgtm
cd "$(dirname "$0")/../token/js" | ||
|
||
npm install | ||
npm run build:program |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not crazy about building a 2nd time.
I think if we remove this from package.json:
"postinstall": "npm run bpf-sdk:update && cd .. && cargo update --manifest-path=Cargo.toml", |
we can remove this 2ndary build. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could remove the program stuff entirely from the npm flow and instead toss up a warning message if the program does not exist and force them to go through do.sh
for al program building/updating, etc...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer that. (I assumed since you didn't do that since last night, you found a reason to keep it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do a follow-up pr for that, was focusing on just getting CI to work and sufficiently cover
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also happy to do that, if you prefer
Add a canonical null address
Instruction
into its own file