Skip to content

Conversation

@grandizzy
Copy link
Collaborator

@grandizzy grandizzy commented Oct 3, 2025

Motivation

Solution

PR Checklist

  • Added Tests
  • Added Documentation (will add subsequently - @o-az)
  • Breaking changes
image

@guidanoli
Copy link

This feature would be very useful to us at cartesi/rollups-contracts. We use Cannon to describe our smart contract deployments, and it runs forge and anvil under-the-hood. If we could have these binaries be distributed and versioned by Node.js project, this would be one less dependency requirement for our devs and users, and we would be able to scope Foundry versions locally! Do you also consider publishing a @foundry-rs/foundry package which includes all these binaries?

@grandizzy
Copy link
Collaborator Author

yes, we could include. Btw forge is already published in npm so you could give ot a try already, this PR tracks publish of the other Foundry artifacts

@o-az o-az force-pushed the o-az/npm-publish-rest branch 3 times, most recently from a47caf4 to a19c5ef Compare October 22, 2025 18:33
@o-az o-az force-pushed the o-az/npm-publish-rest branch from 6c57146 to cd45f3a Compare October 28, 2025 03:48
@o-az o-az marked this pull request as ready for review October 28, 2025 03:48
@o-az o-az force-pushed the o-az/npm-publish-rest branch from 733fb07 to 9273fd8 Compare October 28, 2025 10:09
@grandizzy grandizzy requested a review from 0xrusowsky October 28, 2025 10:18
@grandizzy
Copy link
Collaborator Author

looking for additional approval as I opened the ticket on behalf of @o-az successful run https://github.com/foundry-rs/foundry/actions/runs/18871258811/job/53850363401 CC @DaniPopes @0xrusowsky @zerosnacks @onbjerg

@o-az o-az marked this pull request as draft October 28, 2025 10:22
@grandizzy grandizzy marked this pull request as ready for review October 28, 2025 11:04
Copy link
Contributor

@0xrusowsky 0xrusowsky left a comment

Choose a reason for hiding this comment

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

i'm not an expert, but i think it looks good.

left some minor cmnts though

Comment on lines +3 to +11
/**
* @typedef {'amd64' | 'arm64'} Arch
* @typedef {'linux' | 'darwin' | 'win32'} Platform
* @typedef {'forge' | 'cast' | 'anvil' | 'chisel'} Tool
* @typedef {'debug' | 'release' | 'maxperf'} Profile
*/

/** @type {readonly Tool[]} */
export const KNOWN_TOOLS = Object.freeze(['forge', 'cast', 'anvil', 'chisel'])
Copy link
Contributor

@0xrusowsky 0xrusowsky Oct 28, 2025

Choose a reason for hiding this comment

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

imo it would be nice to centralize all supported platforms in a const with something like:

export const SUPPORTED_PLATFORMS = Object.freeze([
    { platform: 'darwin', arch: 'arm64' },
    { platform: 'darwin', arch: 'amd64' },
    { platform: 'linux', arch: 'arm64' },
    { platform: 'linux', arch: 'amd64' },
    { platform: 'win32', arch: 'amd64' },
  ])

then we could easily add platform validation helpers:

export function isPlatformSupported(platform, arch)

and dynamically generate the CI matrix, ensuring that it is in sync with the supported platforms (rather than manually declaring the huge matrix as we currently do)

Copy link
Collaborator

Choose a reason for hiding this comment

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

planning to address this in a subsequent PR. Specifically, centralizing all references to tools, architectures, etc. in one location vs. in multiple locations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sgtm, @0xrusowsky wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

np, good for me

@grandizzy grandizzy requested a review from 0xrusowsky October 30, 2025 10:15
Copy link
Contributor

@0xrusowsky 0xrusowsky left a comment

Choose a reason for hiding this comment

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

lgtm, pending follow-up PR as discussed.

would be also nice to have another reviewer's approval before merging though

@grandizzy grandizzy added this pull request to the merge queue Oct 30, 2025
Merged via the queue into master with commit dc1d21b Oct 30, 2025
36 checks passed
@grandizzy grandizzy deleted the o-az/npm-publish-rest branch October 30, 2025 12:19
@github-project-automation github-project-automation bot moved this from In Progress to Done in Foundry Oct 30, 2025
@rplusq rplusq mentioned this pull request Oct 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants