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

Bump GB hash and update package-lock.json, plus fix Memory issue on CI #2960

Merged
merged 23 commits into from
Jan 7, 2021

Conversation

hypest
Copy link
Contributor

@hypest hypest commented Jan 5, 2021

PR to bump the Gutenberg submodule hash and update the package-lock.json file, mitigating the lock file issue spotted by the Dependabot PR.

To test:

  • CI should be green across the board, including the extended e2e test job

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@hypest
Copy link
Contributor Author

hypest commented Jan 5, 2021

All tests green except the Correctness check, which failed with more changes detected in package-lock.json. Will run another npm install and commit.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jan 5, 2021

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@ceyhun
Copy link
Contributor

ceyhun commented Jan 5, 2021

After removing and re-adding @wordpress/eslint-plugin to fix missing plugin errors there were still import/no-unresolved and import/no-extraneous-dependencies errors on CI. This is because metro doesn't resolve packages the same way eslint-plugin-import does.

Tried using metro-resolver as a custom eslint-plugin-import resolver, but it didn't work, as the eslint-plugin-import resolvers use different function parameters than metro-resolver.

Finally I managed to resolve all the packages with eslint-import-resolver-node, by adding gutenberg, gutenberg/node_modules to the folders to be looked in for packages.

But all the errors were now import/no-extraneous-dependencies. This requires adding various dependencies to gutenberg-mobile's package.json. But this doesn't make sense as we have already the whole gutenberg repo as a dependency. So I added gutenberg/package.json as a path to also get checked for imported packages. Now everything should be green.

Side note: Seems like both the rules are disabled for React Native code in Gutenberg.

package.json Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
@fluiddot
Copy link
Contributor

fluiddot commented Jan 5, 2021

Not sure why but when I run npm install the package-lock.json changes, did it happen to you @ceyhun?

Screenshot 2021-01-05 at 18 17 05

@ceyhun
Copy link
Contributor

ceyhun commented Jan 6, 2021

Not sure why but when I run npm install the package-lock.json changes, did it happen to you @ceyhun?

This happened some time ago, but doesn't happen anymore. I think it could be related to node/npm version mismatch. Can you please try again after running nvm install --latest-npm inside gutenberg/.

@ceyhun ceyhun requested a review from fluiddot January 6, 2021 14:04
@fluiddot
Copy link
Contributor

fluiddot commented Jan 6, 2021

Not sure why but when I run npm install the package-lock.json changes, did it happen to you @ceyhun?

This happened some time ago, but doesn't happen anymore. I think it could be related to node/npm version mismatch. Can you please try again after running nvm install --latest-npm inside gutenberg/.

I've cleared the node_modules folders and now when running npm install the file didn't change, thanks!

Copy link
Contributor

@fluiddot fluiddot 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 afraid I'm getting the import/no-unresolved error again, however before the latest changes it was working, @ceyhun are you getting it too?

By the way, I saw that one of the PR checks Check Correctness is failing, looks like the machine that is running the process runs out of memory 🤔 :
Error: ENOMEM: not enough memory, read

@fluiddot
Copy link
Contributor

fluiddot commented Jan 7, 2021

I'm afraid I'm getting the import/no-unresolved error again, however before the latest changes it was working, @ceyhun are you getting it too?

I've run npm run lint and it's working, I think the error I saw was caused by the ESLint plugin in VSCode 🙇 .

@ceyhun
Copy link
Contributor

ceyhun commented Jan 7, 2021

I'm afraid I'm getting the import/no-unresolved error again, however before the latest changes it was working, @ceyhun are you getting it too?

Did you try running npm run build:packages inside gutenberg/ before running npm run lint?

By the way, I saw that one of the PR checks Check Correctness is failing, looks like the machine that is running the process runs out of memory 🤔 :
Error: ENOMEM: not enough memory, read

I saw it too, this could be a problem :( To debug it I rerun the job with SSH, but it passed this time. It means that this job could be flaky. I would like to reproduce this again and then solve it for good.

@fluiddot
Copy link
Contributor

fluiddot commented Jan 7, 2021

Did you try running npm run build:packages inside gutenberg/ before running npm run lint?

Good point!. Yeah I think that was the cause of the issue indeed because if I run npm run clear:packages and npm run lint I get the original errors. Running npm run build:packages before npm run lint works like charm, thanks!

@ceyhun
Copy link
Contributor

ceyhun commented Jan 7, 2021

This might need another look now @fluiddot. Managed to fix the memory error by using machine instead of docker as executor, which has 7.5GB instead of 4GB of RAM.
The last 3 builds have passing Check Correctness jobs. Feel free to re-run all the jobs to test another time.

@ceyhun ceyhun requested a review from fluiddot January 7, 2021 16:08
@fluiddot
Copy link
Contributor

fluiddot commented Jan 7, 2021

This might need another look now @fluiddot. Managed to fix the memory error by using machine instead of docker as executor, which has 7.5GB instead of 4GB of RAM.
The last 3 builds have passing Check Correctness jobs. Feel free to re-run all the jobs to test another time.

Good job! I'll re-run them one more time to double check.

I was checking the environment used in Gutenberg PR checks for building the packages and I saw that there we're using GitHub hosted runners which have 7 GB of RAM so probably we need that amount of RAM for this process.

Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

Check Correctness check passed 🎊 !

@ceyhun ceyhun merged commit 1acc6e7 into develop Jan 7, 2021
@ceyhun ceyhun deleted the bump_gb_ref_jan_5_2021 branch January 7, 2021 18:00
@ceyhun ceyhun added the Tooling label Jan 7, 2021
@hypest
Copy link
Contributor Author

hypest commented Jan 8, 2021

Thanks for the added work on this @ceyhun and for the review @fluiddot ! ❤️

@hypest hypest changed the title Bump Gutenberg hash and update package-lock.json Bump GB hash and update package-lock.json, plus fix Memory issue on CI Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants