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

feat(weaver): add build script and fix minor issues #2971

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

outSH
Copy link
Contributor

@outSH outSH commented Jan 5, 2024

  • Add a bash script in tools to build all the weaver packages in single command.
  • Export getMSPConfigurations from fabric interop sdk (is imported in fabric cli, was causing compilation error on node 20)
  • Move bash shebang to the top of check-nodes-status. On ubuntu 20 having this below was causing the file to be interpreted as sh (resulting in error)
  • Update package-local.json files to be more inline with package.json.

Pull Request Requirements

  • Rebased onto upstream/main branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.
  • Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
  • Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

  • Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
  • Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.

@outSH
Copy link
Contributor Author

outSH commented Jan 5, 2024

Hello @VRamakrishna @sandeepnRES, please review :)

More context on changes:

ad Add a bash script in tools to build all the weaver packages in single command.

  • I've wanted to have a simple run-and-leave-for-coffee command for sanity checks of weaver build while I was working on TS upgrade PR, and didn't find anything suitable in weaver dir, so I've created this script - please double check if process is right and I haven't skip any packages. As noted in the comment I'm clearing node_modules in weaver because building it after cacti was causing some errors.

ad Update package-local.json files to be more inline with package.json.

  • My understanding is that this file should be similar to package.json but used to build outside of cacti monorepo context (please correct me if I'm wrong).
  • My intention was to add minimal nodejs / npm requriements (as done previously in package.json) to silent some warnings in newer node, but in the end I've noticed that there are more discrepancies between these two files. So, I've sync these files, except for:
    • references to local weaver packages in package-local.json
    • eslint-plugin-prettier which is 3.4.1 in package-local.json but 5.0.0 in package.json (upgrade caused compilation error, it's a way it already was done before my changes).

outSH added a commit to outSH/cactus that referenced this pull request Jan 5, 2024
- Upgrade typescript to 5.3.3
- Fix minor compilation issues where problems occurred.

Closes hyperledger-cacti#2700

Depends on hyperledger-cacti#2971

Signed-off-by: Michal Bajer <[email protected]>
@sandeepnRES
Copy link
Contributor

sandeepnRES commented Jan 8, 2024

Hello @VRamakrishna @sandeepnRES, please review :)

More context on changes:

ad Add a bash script in tools to build all the weaver packages in single command.

* I've wanted to have a simple run-and-leave-for-coffee command for sanity checks of weaver build while I was working on TS upgrade PR, and didn't find anything suitable in weaver dir, so I've created this script - please double check if process is right and I haven't skip any packages. As noted in the comment I'm clearing node_modules in weaver because building it after cacti was causing some errors.

ad Update package-local.json files to be more inline with package.json.

* My understanding is that this file should be similar to package.json but used to build outside of cacti monorepo context (please correct me if I'm wrong).

* My intention was to add minimal nodejs / npm requriements (as done previously in package.json) to silent some warnings in newer node, but in the end I've noticed that there are more discrepancies between these two files. So, I've sync these files, except for:
  
  * references to local weaver packages in `package-local.json`
  * `eslint-plugin-prettier` which is `3.4.1` in `package-local.json` but `5.0.0` in package.json (upgrade caused compilation error, it's a way it already was done before my changes).

Thanks @outSH for the PR. Looks good to me just one thing you missed all the go packages (except one protos-go is there), not sure if it was intentional. Otherwise the PR looks good to me, given that CI tests are passing even after that package-local.json tests. So if you can confirm about go packages, I can approve the PR.

And to correct the package-local.json files are for building locally i.e. without not depending on the packages published in github. The default package.json files depends upon published github packages.
And that's the difference for every make build and make build-local in each module.

@outSH
Copy link
Contributor Author

outSH commented Jan 9, 2024

Thanks @outSH for the PR. Looks good to me just one thing you missed all the go packages (except one protos-go is there), not sure if it was intentional. Otherwise the PR looks good to me, given that CI tests are passing even after that package-local.json tests. So if you can confirm about go packages, I can approve the PR.

And to correct the package-local.json files are for building locally i.e. without not depending on the packages published in github. The default package.json files depends upon published github packages. And that's the difference for every make build and make build-local in each module.

@sandeepnRES Well, at first I wanted to just build basic scenarios, and then I wanted to make it into build script and I've forgot about go sdk (to be precise, you mean sdks/fabric/go-sdk and samples/fabric/go-cli?). When try to build go-cli now, I run into following error:

rm -rf ./bin
rm -rf vendor
go mod edit -replace github.com/hyperledger/cacti/weaver/common/protos-go/v2=../../../common/protos-go/
go mod edit -replace github.com/hyperledger/cacti/weaver/sdks/fabric/go-sdk/v2=../../../sdks/fabric/go-sdk/
go mod vendor
go: updates to go.mod needed; to update it:
        go mod tidy
make: *** [Makefile:15: run-vendor] Error 1

It seem to be caused by update of go-sdk dependencies in https://github.com/hyperledger/cacti/pull/2901. After go mod tidy the updates are applied in this go.mod as well, but I don't see an easy way to undo them (and keeping them breaks regular build based on remote version of sdk). I think the easy way would be to release latest go-sdk and use it here, but on the other hand would be nice to make this work even after dependencies are updated. Any suggestions? I don't really know golang and it's module system to well

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@outSH I like this a lot personally, but also I'll defer to @VRamakrishna and @sandeepnRES for final approval just to be sure they are OK with it too!

@sandeepnRES
Copy link
Contributor

Thanks @outSH for the PR. Looks good to me just one thing you missed all the go packages (except one protos-go is there), not sure if it was intentional. Otherwise the PR looks good to me, given that CI tests are passing even after that package-local.json tests. So if you can confirm about go packages, I can approve the PR.
And to correct the package-local.json files are for building locally i.e. without not depending on the packages published in github. The default package.json files depends upon published github packages. And that's the difference for every make build and make build-local in each module.

@sandeepnRES Well, at first I wanted to just build basic scenarios, and then I wanted to make it into build script and I've forgot about go sdk (to be precise, you mean sdks/fabric/go-sdk and samples/fabric/go-cli?). When try to build go-cli now, I run into following error:

rm -rf ./bin
rm -rf vendor
go mod edit -replace github.com/hyperledger/cacti/weaver/common/protos-go/v2=../../../common/protos-go/
go mod edit -replace github.com/hyperledger/cacti/weaver/sdks/fabric/go-sdk/v2=../../../sdks/fabric/go-sdk/
go mod vendor
go: updates to go.mod needed; to update it:
        go mod tidy
make: *** [Makefile:15: run-vendor] Error 1

It seem to be caused by update of go-sdk dependencies in #2901. After go mod tidy the updates are applied in this go.mod as well, but I don't see an easy way to undo them (and keeping them breaks regular build based on remote version of sdk). I think the easy way would be to release latest go-sdk and use it here, but on the other hand would be nice to make this work even after dependencies are updated. Any suggestions? I don't really know golang and it's module system to well

Ah okay, I'll approve this PR, we can do that in another PR, I'll look into this why this is happening, ideally it should not

@sandeepnRES sandeepnRES force-pushed the weaver_build_script_pr branch from 8a63530 to 19bb0a1 Compare January 17, 2024 17:49
@outSH outSH force-pushed the weaver_build_script_pr branch from 19bb0a1 to 8cfd6db Compare January 18, 2024 11:30
@outSH outSH requested a review from petermetz January 18, 2024 11:30
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

LGTM!

- Add a bash script in tools to build all the weaver packages in single command.
- Export `getMSPConfigurations` from fabric interop sdk
    (is imported in fabric cli, was causing compilation error on node 20)
- Move bash shebang to the top of `check-nodes-status`. On ubuntu 20 having
    this below was causing the file to be interpreted as sh (resulting in error)
- Update `package-local.json` files to be more inline with `package.json`.

Signed-off-by: Michal Bajer <[email protected]>
@petermetz petermetz force-pushed the weaver_build_script_pr branch from 8cfd6db to 5e1b7dd Compare January 19, 2024 17:43
@petermetz petermetz enabled auto-merge (rebase) January 19, 2024 17:43
@petermetz petermetz merged commit 6d4fd00 into hyperledger-cacti:main Jan 19, 2024
131 of 147 checks passed
petermetz pushed a commit to outSH/cactus that referenced this pull request Jan 19, 2024
- Upgrade typescript to 5.3.3
- Fix minor compilation issues where problems occurred.

Closes hyperledger-cacti#2700

Depends on hyperledger-cacti#2971

Signed-off-by: Michal Bajer <[email protected]>
petermetz pushed a commit to outSH/cactus that referenced this pull request Jan 19, 2024
- Upgrade typescript to 5.3.3
- Fix minor compilation issues where problems occurred.
- Upgraded the protoc-gen-ts package to 0.8.7 which supports
typescript 5.x whereas the previously used version only
supported typescript 4.9 or earlier. This was causing the
yarn codegen script to fail (the code generator script)

Update from Peter: Bumped up protoc-gen-ts to latest versionl.

Closes hyperledger-cacti#2700

Depends on hyperledger-cacti#2971

Signed-off-by: Michal Bajer <[email protected]>
Signed-off-by: Peter Somogyvari <[email protected]>
petermetz pushed a commit that referenced this pull request Jan 19, 2024
- Upgrade typescript to 5.3.3
- Fix minor compilation issues where problems occurred.
- Upgraded the protoc-gen-ts package to 0.8.7 which supports
typescript 5.x whereas the previously used version only
supported typescript 4.9 or earlier. This was causing the
yarn codegen script to fail (the code generator script)

Update from Peter: Bumped up protoc-gen-ts to latest versionl.

Closes #2700

Depends on #2971

Signed-off-by: Michal Bajer <[email protected]>
Signed-off-by: Peter Somogyvari <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants