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

chore(cacti): use lerna 8.1 instead of lerna lite #3369

Closed

Conversation

jenniferlianne
Copy link

First step of ci refactoring.


Please see ./epics/2024-06-ci-refactoring.md in
this commit for ci refactoring plan.

Primary Change:
 - switch from using lerna lite to lerna 8.1

 Secondary Changes:
 - add ci refactoring plan markdown file
 - lerna.json: remove useWorkspaces, which is not supported by lerna 8.1
 - introduce package.lock, a new file generated
 - package.json: increase memory used when building
 - yarn.lock: introduce lerna 8.1

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.

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.

@jenniferlianne

I tried to run yarn configure locally but it crashes (logs below). Any idea how to fix it?

yarn-configure-crash-lerna-v8.1.log

[11:37:05 PM] Building project '/home/peter/a/cacti-upstream/packages/cactus-plugin-persistence-fabric/tsconfig.json'...

lerna notice cli v8.1.5
lerna ERR! TypeError: Cannot use 'in' operator to search for 'extends' in undefined
lerna ERR!     at applyExtends (/home/peter/a/cacti-upstream/node_modules/lerna/dist/index.js:2429:17)
lerna ERR!     at Object.transform (/home/peter/a/cacti-upstream/node_modules/lerna/dist/index.js:2694:28)
lerna ERR!     at load (/home/peter/a/cacti-upstream/node_modules/cosmiconfig/dist/ExplorerSync.js:20:32)
lerna ERR!     at emplace (/home/peter/a/cacti-upstream/node_modules/cosmiconfig/dist/util.js:12:20)
lerna ERR!     at ExplorerSync.load (/home/peter/a/cacti-upstream/node_modules/cosmiconfig/dist/ExplorerSync.js:23:42)
lerna ERR!     at ExplorerSync.search (/home/peter/a/cacti-upstream/node_modules/cosmiconfig/dist/ExplorerSync.js:30:33)
lerna ERR!     at #resolveLernaConfig (/home/peter/a/cacti-upstream/node_modules/lerna/dist/index.js:2698:27)
lerna ERR!     at new _Project (/home/peter/a/cacti-upstream/node_modules/lerna/dist/index.js:2560:78)
lerna ERR! ENOPROJECT Lerna Project not initialized!
/home/peter/a/cacti-upstream/node_modules/lerna/dist/index.js:3181
          throw new ValidationError("ENOPROJECT", "Lerna Project not initialized!");
                ^

ValidationError: Lerna Project not initialized!
    at get project [as project] (/home/peter/a/cacti-upstream/node_modules/lerna/dist/index.js:3181:17)
    at /home/peter/a/cacti-upstream/node_modules/lerna/dist/index.js:3158:35 {
  prefix: 'ENOPROJECT'
}

Node.js v20.11.1
ERROR: "build:dev:backend:postbuild" exited with 1.

One more thing: No package-lock.json please. We use yarn so the only lock file allowed/needed is yarn.lock

@jenniferlianne jenniferlianne force-pushed the use_lerna_8_1 branch 2 times, most recently from fa560db to 5d7cf16 Compare August 2, 2024 18:12
First step of ci refactoring.

Please see ./epics/2024-06-ci-refactoring.md in
this commit for ci refactoring plan.

Primary Change:
 - switch from using lerna lite to lerna 8.1

 Secondary Changes:
 - add ci refactoring plan markdown file
 - lerna.json: remove useWorkspaces, which is not
supported by lerna 8.1
 - introduce package.lock, a new file generated
 - package.json: increase memory used when
   building
 - yarn.lock: introduce lerna 8.1

Signed-off-by: Jennifer Bell <[email protected]>

remove package.lock
@jenniferlianne
Copy link
Author

jenniferlianne commented Aug 2, 2024

I checked it out and ran yarn configure, and it works for me. npm run configure is also working in CI. Is it possible your problem is local?

I removed package-lock.json

@outSH
Copy link
Contributor

outSH commented Aug 6, 2024

@petermetz I've tested this on my machine as well and everything seem to be working fine. I've removed node_modules for a clean start and both build and confiure commands finished without any error

@petermetz
Copy link
Contributor

@jenniferlianne @outSH It's still broken every which way for me. I think I know what the issue is and I sent a PR to lerna: lerna/lerna#4062. Hopefully they agree and then we can merge this with the new release once it's out. Until then I'd love to keep this not merged because dozens of other contributors who are actively working on branches could be affected by the same breakage as I am which could lead to hundreds of hours of person-hours lost if we break the build for them on main and they don't immediately figure out what's wrong with it.

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.

Let's see what happens with the PR I linked to above. @jenniferlianne Please re-request review once the new release is out with a fix (if they'll accept the fix) and you've changed the versions here to have the upgraded version.
Then please test it by running the API server config generator script and then running yarn configure to see if the breakage still happens or not.

"packageManager": "[email protected]"
"packageManager": "[email protected]",
"dependencies": {
"lerna": "^8.1.5"
Copy link
Contributor

Choose a reason for hiding this comment

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

@jenniferlianne One more thing I just notced: Please put this under devDependencies. It doesn't really matter which one it is since all dependencies are dev dependencies in the root package.json anyway but still it's much better to have them organized into a single section.

One more thing: Please pin it to an exact version, auto-upgrades open us up to nasty software-supply-chain attacks.

@petermetz
Copy link
Contributor

petermetz commented Aug 9, 2024

@jenniferlianne In the meantime could you please confirm that the release publishing scripts (the steps in RELEASE_MANAGEMENT.md) work (both the regular and the canary one?)
The lerna repo has examples of how to direct it to a local npm registry (verdaccio) so you don't need to worry about not being able to publish packages to npmjs.com

@@ -18,7 +18,6 @@
],
"version": "2.0.0-rc.3",
"npmClient": "yarn",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it the case that with lerna we do not need this configuration option (as vs lerna lite)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@RafaelAPB Good question, I'm not sure TBH. It could be that lerna ignores this option, but I didn't look up their docs. Maybe @jenniferlianne knows.

Copy link
Contributor

@RafaelAPB RafaelAPB left a comment

Choose a reason for hiding this comment

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

This is great!
I've run configure and it fails for me (macos).
LGTM - please make sure to address Peter's comments and include his fix.

@petermetz
Copy link
Contributor

@jenniferlianne Great news! The fix for lerna has been merged: lerna/lerna#4062
Now we just need them to issue a new release (not sure when but I'm hoping soon) and then we can upgrade to that version in this PR and I'll be good to go assuming that the manual testing [1] I requested was also successful.

[1] https://github.com/hyperledger/cacti/pull/3369#issuecomment-2278871577

@petermetz
Copy link
Contributor

@jenniferlianne Lerna still hasn't issued a new release unfortunately, but in the meantime what you could do to move it along is to rebase the branch onto upstream/main and specify the dependency version (for lerna) with the special syntax that points to a github repository's main branch (Lerna's GitHub repository in this case). With all that done, we could finally merge this and just update the dependency version later once they do issue an official release with my hot-fix in it.

@petermetz
Copy link
Contributor

Closing due to inactivity for now. Please feel free to reopen if there's additional bandwidth available to take it over the finish line.

@petermetz petermetz closed this Oct 18, 2024
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.

4 participants