Skip to content

chore: improve ci #150

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

Merged
merged 15 commits into from
Feb 26, 2024
Merged

chore: improve ci #150

merged 15 commits into from
Feb 26, 2024

Conversation

Cyberhan123
Copy link
Contributor

@Cyberhan123 Cyberhan123 commented Jan 19, 2024

This PR is mainly to improve the construction scope of ci, mainly for windows and mac. And I belive most of the people who use linux are developers, most of them are more professional than me.

There are also some reasons:

  1. There are also compatibility issues with glibc.
  2. There are many linux distributions.
  3. cuda construction will consume a lot of time (I also lack experience in implementing CUDA on Linux).

Of course, if someone is willing, they can continue to improve in subsequent PRs. I have no objection.

message("Build shared library")
set(BUILD_SHARED_LIBS OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks very counter intuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hahaha, but this is the key to packaging all the code into a dynamic library, which will help me perform black magic in golang.
Another very important benefit is that you don’t have to worry about what version of ggml (avx2? cuda? commit hash?) it is. You only need to replace the dynamic library. Ideally, I can just download the dynamic library in the desktop app. Errors about sd.cpp can be fixed without having to worry about other dynamic libraries.

@Cyberhan123
Copy link
Contributor Author

Cyberhan123 commented Jan 19, 2024

Hi @leejet , I have completed the upgrade to ci. It's waiting to be merged. But one thing I want to explain is that this build logic contains some if judgment logic. They are ignored in this PR when the ci run, so there may be some problems when merged into the master. Of course I will follow up and make changes.

@Cyberhan123 Cyberhan123 requested a review from Green-Sky January 19, 2024 12:35
@leejet
Copy link
Owner

leejet commented Jan 29, 2024

Have you tested it in your master branch?

@Cyberhan123
Copy link
Contributor Author

Cyberhan123 commented Feb 3, 2024

Have you tested it in your master branch?

@leejet Yes, I've tried this on my main branch, and with some adjustments, the build product is now being generated normally.
here is the latest release on my main branch: https://github.com/Cyberhan123/stable-diffusion.cpp/releases/tag/master-bd56734

@leejet
Copy link
Owner

leejet commented Feb 24, 2024

I think it's best to include the cudart dependency for Windows in the packaged build, just like what llama.cpp did, as shown in https://github.com/ggerganov/llama.cpp/releases.

@leejet
Copy link
Owner

leejet commented Feb 24, 2024

There is another issue:
stable-diffusion.h is a pure C header file, so it should not include anything C++. Therefore, the declarations of sd_basename and preprocess_canny need to be removed from this file. If your project relies on these two functions, I suggest implementing them separately.

@Cyberhan123
Copy link
Contributor Author

Cyberhan123 commented Feb 25, 2024

There is another issue: stable-diffusion.h is a pure C header file, so it should not include anything C++. Therefore, the declarations of sd_basename and preprocess_canny need to be removed from this file. If your project relies on these two functions, I suggest implementing them separately.

The build products of CI are dynamic libraries and CLI programs. If the dynamic library does not expose the two functions sd_basename and preprocess_canny, they need to be migrated to the CLI program.
In other words, the cli program needs these two methods of the dynamic library.

@Cyberhan123
Copy link
Contributor Author

I think it's best to include the cudart dependency for Windows in the packaged build, just like what llama.cpp did, as shown in https://github.com/ggerganov/llama.cpp/releases.

There's certainly nothing wrong with that, I can make adjustments

@leejet
Copy link
Owner

leejet commented Feb 25, 2024

The build products of CI are dynamic libraries and CLI programs. If the dynamic library does not expose the two functions sd_basename and preprocess_canny, they need to be migrated to the CLI program.

There is indeed an issue here. I merged the latest changes from the master branch and made some adjustments.

@Cyberhan123
Copy link
Contributor Author

@leejet
Copy link
Owner

leejet commented Feb 26, 2024

Great! Thank you for your contribution.

@leejet leejet merged commit b7870a0 into leejet:master Feb 26, 2024
rmatif pushed a commit to rmatif/stable-diffusion.cpp that referenced this pull request Apr 8, 2025
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