-
Notifications
You must be signed in to change notification settings - Fork 75
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
Use upstream Manager. #912
base: main
Are you sure you want to change the base?
Conversation
3ec307e
to
24a7101
Compare
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
123cba3
to
7f736bb
Compare
Rebased and tested again. @webfiltered tests are failing because I updated the version to 0.4.22 and the screenshots don't match on the error page anymore. How do I update that? |
Process is currently:
yarn run test:e2e:update
All screenshot diffs are overwritten by playwright at once - doing a partial diff requires discarding any updated expectations before committing. Edit: Adding this to testing README. Done. |
Or.. we could just resolve the problem at the source. |
d5dffd8
to
c713dda
Compare
@webfiltered Closed this by accident. I rebased and tested. It should fix a few issues with current manager, including migrating custom nodes. Can you PTAL when you have time? |
// Match the original case: 2 packages (uv + toml) | Added in https://github.com/ltdrdata/ComfyUI-Manager/commit/816a53a7b1a057af373c458ebf80aaae565b996b | ||
const twoPackages = /\bWould install 2 packages(\s+\+ (toml|uv)==[\d.]+){2}\s*$/; | ||
// Match the new case: 1 package (chardet) | Added in https://github.com/ltdrdata/ComfyUI-Manager/commit/60a5e4f2614c688b41a1ebaf0694953eb26db38a | ||
const onePackage = /\bWould install 1 package\s+\+ chardet==[\d.]+\s*$/; | ||
|
||
return twoPackages.test(output) || onePackage.test(output); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Match the original case: 2 packages (uv + toml) | Added in https://github.com/ltdrdata/ComfyUI-Manager/commit/816a53a7b1a057af373c458ebf80aaae565b996b | |
const twoPackages = /\bWould install 2 packages(\s+\+ (toml|uv)==[\d.]+){2}\s*$/; | |
// Match the new case: 1 package (chardet) | Added in https://github.com/ltdrdata/ComfyUI-Manager/commit/60a5e4f2614c688b41a1ebaf0694953eb26db38a | |
const onePackage = /\bWould install 1 package\s+\+ chardet==[\d.]+\s*$/; | |
return twoPackages.test(output) || onePackage.test(output); | |
// Match the new case: 1 package (chardet) | Added in https://github.com/ltdrdata/ComfyUI-Manager/commit/60a5e4f2614c688b41a1ebaf0694953eb26db38a | |
const latest = /\bWould install 1 package\s+\+ chardet==[\d.]+\s*$/; | |
// Match upgrade from <0.4.18: 2 additional packages (uv + toml) | Added in https://github.com/ltdrdata/ComfyUI-Manager/commit/816a53a7b1a057af373c458ebf80aaae565b996b | |
const allPackages = /\bWould install 3 packages(\s+\+ (toml|uv|chardet)==[\d.]+){2}\s*$/; | |
return latest.test(output) || allPackages.test(output); |
If you're updating from <0.4.18 to this commit, you should need all three.. however.. possibly a better idea in a second..
// Match the original case: 2 packages (uv + toml) | Added in https://github.com/ltdrdata/ComfyUI-Manager/commit/816a53a7b1a057af373c458ebf80aaae565b996b | ||
const twoPackages = /\bWould install 2 packages(\s+\+ (toml|uv)==[\d.]+){2}\s*$/; | ||
// Match the new case: 1 package (chardet) | Added in https://github.com/ltdrdata/ComfyUI-Manager/commit/60a5e4f2614c688b41a1ebaf0694953eb26db38a | ||
const onePackage = /\bWould install 1 package\s+\+ chardet==[\d.]+\s*$/; | ||
|
||
return twoPackages.test(output) || onePackage.test(output); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Match the original case: 2 packages (uv + toml) | Added in https://github.com/ltdrdata/ComfyUI-Manager/commit/816a53a7b1a057af373c458ebf80aaae565b996b | |
const twoPackages = /\bWould install 2 packages(\s+\+ (toml|uv)==[\d.]+){2}\s*$/; | |
// Match the new case: 1 package (chardet) | Added in https://github.com/ltdrdata/ComfyUI-Manager/commit/60a5e4f2614c688b41a1ebaf0694953eb26db38a | |
const onePackage = /\bWould install 1 package\s+\+ chardet==[\d.]+\s*$/; | |
return twoPackages.test(output) || onePackage.test(output); | |
// Match the original case: 2 packages (uv + toml) | Added in https://github.com/ltdrdata/ComfyUI-Manager/commit/816a53a7b1a057af373c458ebf80aaae565b996b | |
// Match the new case: 1 package (chardet) | Added in https://github.com/ltdrdata/ComfyUI-Manager/commit/60a5e4f2614c688b41a1ebaf0694953eb26db38a | |
const anyCombination = /\bWould install [1-3] packages(\s+\+ (toml|uv|chardet)==[\d.]+){1,3}\s*$/; | |
return anyCombination.test(output); |
If they downloaded a custom node that added just one of the required packages, we should still just auto-update, right?
if (fs.existsSync(managerPath)) { | ||
await fs.promises.rm(managerPath, { recursive: true, force: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would really like to limit our use of node:fs
to CI scripts and dev stuff. Unless node / Electron is silently wrapping this in some real multi-threaded framework magic, this can block the entire main process from responding.
node:fs/promises
is the way (we have our own wrappers for fs.access
)
ltdrdata/ComfyUI-Manager@488f023
restore-to
to fix importing custom nodes.┆Issue is synchronized with this Notion page by Unito