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

Fix Build Command (supersedes #4848) #4861

Closed
wants to merge 17 commits into from

Conversation

spacesailor24
Copy link
Contributor

Original PR: #4848, this fixed some merge conflicts with package-lock.json

Something to note is a ran npm i which apparently fixed the version number of all packages, changing them from 1.7.1-rc.0 to 1.7.1

@render
Copy link

render bot commented Mar 17, 2022

@spacesailor24 spacesailor24 self-assigned this Mar 17, 2022
@spacesailor24 spacesailor24 added the 1.x 1.0 related issues label Mar 17, 2022
@spacesailor24 spacesailor24 mentioned this pull request Mar 17, 2022
14 tasks
@coveralls
Copy link

coveralls commented Mar 17, 2022

Pull Request Test Coverage Report for Build 2002666498

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 265 unchanged lines in 7 files lost coverage.
  • Overall coverage increased (+2.3%) to 74.497%

Files with Coverage Reduction New Missed Lines %
packages/web3-core-requestmanager/src/jsonrpc.js 1 70.0%
packages/web3-core-helpers/src/formatters.js 7 85.42%
packages/web3-core-helpers/src/errors.js 29 1.56%
packages/web3-utils/src/soliditySha3.js 34 3.43%
packages/web3-utils/src/index.js 43 31.38%
packages/web3-utils/src/utils.js 44 9.66%
packages/web3-eth-accounts/src/index.js 107 23.67%
Totals Coverage Status
Change from base Build 1999712194: 2.3%
Covered Lines: 3238
Relevant Lines: 4102

💛 - Coveralls

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jdevcs jdevcs left a comment

Choose a reason for hiding this comment

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

We should not make changes in package-lock.json in this PR, as scope of issue is for improvement in build commands

@wbt
Copy link
Contributor

wbt commented Mar 17, 2022

@jdevcs this PR introduces a new dependency on rimraf for the rm -rf functionality, so package-lock.json does need to be on the changed files list.

spacesailor24 and others added 2 commits March 17, 2022 20:34
Co-authored-by: Junaid <86780488+jdevcs@users.noreply.github.com>
@spacesailor24 spacesailor24 requested a review from jdevcs March 18, 2022 11:49
@@ -127,6 +127,7 @@
"mocha": "^6.2.3",
"nyc": "^14.1.1",
"pify": "^4.0.1",
"rimraf": "^3.0.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"rimraf": "^3.0.2",

"test:e2e:ganache:core": "bash ./scripts/e2e.ganache.core.sh",
"test:e2e:gnosis:dex": "bash ./scripts/e2e.gnosis.dex.sh",
"ci": "bash ./scripts/ci.sh",
"cov:clean": "rimraf .nyc_output && rimraf coverage",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"cov:clean": "rimraf .nyc_output && rimraf coverage",
"cov:clean": "rm -rf .nyc_output; rm -rf coverage",

Copy link
Contributor

Choose a reason for hiding this comment

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

The suggested change actively reduces cross-platform compatibility. Can you explain why it's important to avoid any changes to package-lock?

Copy link
Contributor

Choose a reason for hiding this comment

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

@wbt We include changes in package-lock files via dedicated and internally initialized PRs only.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is now formally an internally initialized PR. Going out of your way to reduce cross-platform compatibility and break the command on some platforms just doesn't seem like the right direction.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wbt We include changes in package-lock files via dedicated and internally initialized PRs only.

Internal and also dedicated PRs for package lock changes. Thanks

Copy link
Contributor

@wbt wbt Apr 1, 2022

Choose a reason for hiding this comment

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

However, the version of this PR which does not change all the package-lock files this one does was rejected at #4848. In theory, I believe that one could be reopened and merged, but @spacesailor24 seems to prefer the version with all the additional package-lock changes while @jdevcs strongly disagrees, and meanwhile the command nominally required to be run for every PR remains broken.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wbt , we discussed this internally that @spacesailor24 will re generate package lock files in this PR, but there are errors when he tried. #4861 (comment)

Our team is occupied in 4.x so could you investigate and share it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jdevcs can you just reopen #4848?

Copy link
Contributor

Choose a reason for hiding this comment

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

Its already merged so couldn't reopen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Replaced with #4939.

Copy link
Contributor

@jdevcs jdevcs left a comment

Choose a reason for hiding this comment

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

@jdevcs this PR introduces a new dependency on rimraf for the rm -rf functionality, so package-lock.json does need to be on the changed files list.

@spacesailor24 we need to remove changes from package lock files in this PR. thanks

@spacesailor24 spacesailor24 requested a review from jdevcs March 21, 2022 09:04
Copy link
Contributor

@avkos avkos left a comment

Choose a reason for hiding this comment

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

rimraf works exactly the same as rm-rf but with cross-platform support. Why do we need to change rimraf to rm -rf? What is it solve?

@spacesailor24 spacesailor24 requested a review from avkos March 23, 2022 17:15
@spacesailor24
Copy link
Contributor Author

spacesailor24 commented Mar 25, 2022

rimraf works exactly the same as rm-rf but with cross-platform support. Why do we need to change rimraf to rm -rf? What is it solve?

@avkos Web3.js supports Windows, OSX, and Linux environments, hence the cross platform support needed for removing files

@spacesailor24
Copy link
Contributor Author

spacesailor24 commented Mar 25, 2022

Okay so I deleted all the package-lock.json files and regenerated them with npm i, and now when running npm run build we're getting the errors:

../../node_modules/@ethereumjs/common/dist/types.d.ts(38,32): error TS1005: ',' expected.
../../node_modules/@ethereumjs/common/dist/types.d.ts(38,58): error TS1005: ',' expected.
../../node_modules/@ethereumjs/common/dist/types.d.ts(40,12): error TS1005: ',' expected.
../../node_modules/@ethereumjs/common/dist/types.d.ts(41,9): error TS1005: ',' expected.
../../node_modules/@ethereumjs/common/dist/types.d.ts(42,12): error TS1005: ',' expected.

@avkos
Copy link
Contributor

avkos commented Mar 29, 2022

rimraf works exactly the same as rm-rf but with cross-platform support. Why do we need to change rimraf to rm -rf? What is it solve?

@avkos Web3.js supports Windows, OSX, and Linux environments, hence the cross platform support needed for removing files

I mean rimraf is better than rm -rf. We need to use it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants