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

Use pre-built static webui libraries #27

Closed
wants to merge 4 commits into from

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Jul 5, 2023

This PR moves towards using the pre-built libraries, like e.g. go-webui does.

Reasons for this were discussed e.g. in #21, #25

The pre-builts are added as repository files, like it's done in other webui repositories. But as mentioned in e.g webui-dev/webui#114, pre-built fiels should be moved out of the repositories mid-term. For this I can submit a solution to handle them via github releases, when a free moment opens up in the coming weeks - first to the main repo and then update v-webui accordingly.

I couldn't test the changes on windows. Please verify that examples build
Edit:
I came to test the changes on linux, macos m1 and windows. So we should be good 🙂. To give a quick glance compiling the text-editor example on windows with v -cg -cc gcc run .\text-editor.v:

Annotation 2023-07-06 132102

Last, I'd like to mention that there comes a drawback when using the static pre-builts. It won't be possible to pass -d webui_log, as the libs would have to be compiled with the debug flag. We could handle this by also including the set of prebuilts with the debug flag. I'd like to hear you opinion on this.

Copy link
Member Author

@ttytm ttytm Jul 5, 2023

Choose a reason for hiding this comment

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

webui.h was updated to it's v2.3 release state.

Copy link
Member Author

Choose a reason for hiding this comment

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

Source files that are not needed anymore were removed.

@AlbertShown
Copy link
Contributor

Big improvement! This will speed up the compiling time. Thank you, @tobealive 👍

the libs would have to be compiled with the debug flag

The release has no debug version. Probably we should start adding the debug version to the release package as many other projects/repo does. Otherwise, I don't have another solution.

@ttytm
Copy link
Member Author

ttytm commented Jul 6, 2023

Big improvement! This will speed up the compiling time. Thank you, @tobealive +1

It's my pleasure 👍

A way of handling releases I had in mind was adding an automated release workflow when you create a release tag. A CI action would compile and upload the debug and release prebuilts to github releases.
A webui wrapper like v-webui could then automatically pull the libraries from the github releases based on it's needs, if they are not already present.

If you think that is a good way, I should find the time to integrate the CI on the core repository and add a sample of pulling the libraries here on this vlang version.

@malisipi
Copy link
Collaborator

malisipi commented Jul 7, 2023

It is looking good, however i need to be sure about platform support before merging it.
Also thanks for your PRs and patience. Since i'm studying for university entrance exam, I don't have much time to review PRs.

@ttytm
Copy link
Member Author

ttytm commented Jul 9, 2023

Thanks for the reply @malisipi. No worries, take you time 👍 .

If - due to the limited time - attaching the executables for the different platforms or something is of any help, let me know.

@ttytm ttytm mentioned this pull request Jul 9, 2023
@malisipi
Copy link
Collaborator

malisipi commented Jul 9, 2023

Compilers Windows Linux MacOS x64 MacOS arm64
TCC No No Not tested Not tested
Clang Yes Yes Not tested Not tested
GCC Yes Yes Not tested Not tested
  • TCC support is broken in Linux, however it's can be ignored since GCC/Clang is working perfectly.
  • TCC support is still broken in Windows, i guess the main reason of that is missing tlhelp32.h header file and it should be fixed when the pre-built library is complied as TCC compatible.
  • I can not comment for MacOS since i don't have one, however it should be fine. (Maybe TCC support could be broken)

@ttytm
Copy link
Member Author

ttytm commented Jul 9, 2023

Thanks for testing!

I can compile with tcc on linux, when you find a moment can you share about your issues on linux?

On windows I can confirm, tcc is the same result as discussed in #21. Using gcc precompiled lib is just the suggested workaround in this issue until tlhelp32.h issue is fixed.

When pre-builts are moved to a more automated handling, adding pre-builts versions with different compilers can also be considered. But afaik a gcc precompiled lib should work with all compilers in the final program.

@ttytm
Copy link
Member Author

ttytm commented Jul 9, 2023

For mac arm, if it gives peace of mind. I can add add screenshots or a short recording of the compilation.

Or adding CI that tests compilation in a separate PR and than rebase. Adding the CI beforehand might be problematic but I would be down to try.

Edit:
I managed to create a build workflow covering several compilation scenarios to get automated reliable info about build status and test it against the submitted branch.

Linux and Mac work with all compilers. For windows, the only working compiler is gcc. The results match with the results I had testing locally.

image

https://github.com/tobealive/v-webui/actions/runs/5506931824

@ttytm ttytm mentioned this pull request Jul 10, 2023
@malisipi
Copy link
Collaborator

malisipi commented Jul 10, 2023

In Linux (and maybe MacOS), V uses another C compiler when TCC was crashed. You can see it with -showcc flag. However it's not big issue and it could be a issue on my device. Also this PR is improving build times/feature support. So TCC support can be ignored for interest of people.

In Windows, Clang is working with a few trick. It can be fixed with disabling warnings with -Wno-everything or fixing these warnings. However, Clang on Windows is throwing a lot of warnings that most C compilers ignores. And disabling warnings is bad for backtrace since V doesn't support backtrace in Clang. Maybe we can add a flag to make Clang working on Windows or informate people about -cflags "-Wno-everything".

@ttytm
Copy link
Member Author

ttytm commented Jul 10, 2023

Didn't knew about those 🤓 thanks for the info @malisipi!

I'll rebase this against main for the ci and do some tests with the new insights tomorrow 👍

@ttytm
Copy link
Member Author

ttytm commented Jul 11, 2023

Adding -showcc while compiling with tcc doesn't reveil anything abnormal. So the tcc issue you experience might really be due to something on your system.

For the tests I could do with clang. It won't compile on windows:

photo_2023-07-11_04-42-53

Screenshot from 2023-07-11 04-38-00

Until a few moments ago I thought there is a also clang compiled Windows webui core. That's why I mentioned that we might just use the clang prebuilt. Realizing there is none for Windows I made a make file for it. But trying to clang compile the core library results in a similar linker error like you see on the screenshots - using the gcc pre-built and clang compiling the app.
Maybe it's llvm-mingw instead of using clang directly that adds clang support?

If clang requires too much of a workaround to be usable, I'd say gcc out of the box for windows is fair enough. And there is still wsl.

Edit:
As little info, I removed the Close tags for the related issues on top of this PR because I didn't knew if want to keep the them open as their topics cover more than just moving towards using pre-builts.

@ttytm
Copy link
Member Author

ttytm commented Aug 23, 2023

Having results available via github releases is probably close enough to not add the prebuilts to the repo as intermediary step but directly handle them via gh releases. I'll update this accordingly when the related upstream changes are done.

@ttytm
Copy link
Member Author

ttytm commented Aug 23, 2023

I found a good approach that can be used already - prior to making a release = to prepare the wrapper with the newest changes. Submitting it after work this night 👍

@ttytm ttytm mentioned this pull request Aug 23, 2023
@ttytm
Copy link
Member Author

ttytm commented Aug 23, 2023

Superseded by #37

@ttytm ttytm closed this Aug 23, 2023
@ttytm ttytm deleted the use-prebuilts branch August 25, 2023 02:46
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.

None yet

3 participants