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

Jump to bottom

CIP-25 build script #160

Merged
merged 2 commits into from
Jan 3, 2023
Merged

CIP-25 build script #160

merged 2 commits into from
Jan 3, 2023

Conversation

rooooooooob
Copy link
Contributor

CIP-25 was used as an experiment to try out the new split crate architecture. The build scripts were adapted to work for this new format. The old build scripts are moved to /scripts/legacy/. This should allow us to have per wasm target npm crates as well. If this works out, we'll adapt the other WASM crates to use this build structure too.

@rooooooooob
CIP-25 build script
8172aa6 
CIP-25 was used as an experiment to try out the new split crate
architecture. The build scripts were adapted to work for this new
format. The old build scripts are moved to `/scripts/legacy/`. This
should allow us to have per wasm target npm crates as well.
If this works out, we'll adapt the other WASM crates to use this build
structure too.
rooooooooob
rooooooooob commented Dec 27, 2022
View reviewed changes
cip25-wasm/package.json Outdated
"rust:check-warnings": "(RUSTFLAGS=\"-D warnings\" cargo +stable build)",
"rust:test": "cargo test",
"js:prepublish": "npm run rust:test && rimraf ./publish && cp -r ./pkg ./publish && cp README.md publish/ && cp LICENSE publish/",
"js:test-publish": "npm run rust:build-nodejs && npm run js:prepublish && node ../scripts/publish-helper cip25 cip25_wasm -nodejs && cd publish",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can remove this one. I just had it here to test out the publishing scripts since I had never used them and wasn't sure if running npm publish would do anything I didn't want to happen.

rooooooooob
rooooooooob commented Dec 27, 2022
View reviewed changes
cip25-wasm/package.json
"license": "MIT",
"repository": {
"type": "git",
"url": "git+https://github.com/dcSpark/cardano-multiplatform-lib.git"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will having multiple npm packages all linking to this repo be fine?

@rooooooooob
Copy link
Contributor Author

The extent of testing I've done with this has been to run the scripts and make sure that the JSON definitions are properly inserted into the resulting .ts file in /pkg/. For the publish ones I ran a new test publish script that doesn't include the npm publish part since I am unfamiliar with publishing NPM packages. The idea is we'd have 1 of these per WASM module. (e.g. crypto-wasm, cip36-wasm, wasm (which is the one that includes everything and for now corresponds to chain but that could change once we add in more functionality like tx builders, etc) etc).

gostkin
gostkin reviewed Dec 29, 2022
View reviewed changes
scripts/publish-helper.js Outdated
oldPkg.files = [
'cardano_multiplatform_lib.asm.js',
...oldPkg.files.filter(file => file !== 'cardano_multiplatform_lib_bg.wasm')
`cardano_multiplatform_lib_${pkgModName}.asm.js`,
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be a constant probably?

@gostkin
Copy link
Contributor

gostkin commented Dec 29, 2022

looks good, not sure what's the best way to test publishing part, other things work fine

@gostkin
Copy link
Contributor

gostkin commented Dec 29, 2022

js:test-publish gives:

node:internal/modules/cjs/loader:936
  throw err;
  ^

Error: Cannot find module './publish/package.json'
Require stack:
- /Users/gostkin/CLionProjects/cardano-multiplatform-lib/scripts/publish-helper.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
    at Function.Module._load (node:internal/modules/cjs/loader:778:27)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/Users/gostkin/CLionProjects/cardano-multiplatform-lib/scripts/publish-helper.js:2:16)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/Users/gostkin/CLionProjects/cardano-multiplatform-lib/scripts/publish-helper.js'
  ]
}

SebastienGllmt
SebastienGllmt reviewed Jan 3, 2023
View reviewed changes
cip25-wasm/package.json
"rust:build-nodejs": "rimraf ./pkg; wasm-pack build --target=nodejs; npm run js:ts-json-gen; wasm-pack pack",
"rust:build-browser": "rimraf ./pkg; wasm-pack build --target=browser; npm run js:ts-json-gen; wasm-pack pack",
"rust:build-web": "rimraf ./pkg; wasm-pack build --target=web; npm run js:ts-json-gen; wasm-pack pack",
"rust:publish": "cargo publish",
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 really want to publish the WASM repo? Or are we instead trying to publish the non-WASM crate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought we wanted to publish the WASM crates to NPM and the rust ones to crates.io? I just copied what we had in the old build script but with a per-crate (or at least per end-user crate) approach. I don't know how the actual process works after that though since it was always done by someone else which is why I asked if there was a problem with having multiple packages referring to one repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, the other publish commands are correct. I was referring specifically to cargo publish, since I assume what we want to publish to crates.io is not the WASM crate, but rather the non-WASM crate. I could be wrong on this though

@SebastienGllmt SebastienGllmt merged commit 5cdd375 into develop Jan 3, 2023
@SebastienGllmt SebastienGllmt deleted the cip25-build-script branch January 3, 2023 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SebastienGllmt SebastienGllmt SebastienGllmt left review comments

@gostkin gostkin gostkin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rooooooooob @gostkin @SebastienGllmt