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

make tracy a lazy dependency #1742

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

Techatrix
Copy link
Member

This allows us to completely get rid of git submodules.

There is just one unfortunate issue which is why I made this a separate Draft from #1741.
When using an old zig version like 0.11.0 you will get this unhelpful error when compiling ZLS.

warning: FileNotFound: /home/techatrix/.cache/zig/p/12202aac930cebaf2b57f443cacc2483478580a72f1316b4f0a720ddd91246fce69d/build.zig
error: FileNotFound

This is because 0.11.0 has no support for dependencies that do not have a build.zig like the tracy repo.
So we either lose our minimum build version check (and gain new issues reports because of it) or we use a tracy wrapper library that provides a build.zig. It would also be possible to wait for the Zig 0.12.0 release since it will become more unlikely that someones tries to compile ZLS with 0.11.0 after that.

Copy link

codecov bot commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (88a352a) 76.62% compared to head (b0f4e1f) 76.62%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1742   +/-   ##
=======================================
  Coverage   76.62%   76.62%           
=======================================
  Files          35       35           
  Lines       10143    10143           
=======================================
  Hits         7772     7772           
  Misses       2371     2371           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BratishkaErik
Copy link
Contributor

It would also be possible to wait for the Zig 0.12.0 release since it will become more unlikely that someones tries to compile ZLS with 0.11.0 after that.

Hello, any chance this is merged (given that both Zig 0.12.0 and ZLS 0.12.0 are now available)?

@Techatrix
Copy link
Member Author

Hello, any chance this is merged (given that both Zig 0.12.0 and ZLS 0.12.0 are now available)?

Any motivation behind this comment or is this just a random question?

I would rather want to import a "zig tracy" package than doing this in ZLS. I started working on that but I am currently working on other things. I could still merge this and change it later.

@BratishkaErik
Copy link
Contributor

Any motivation behind this comment or is this just a random question?

It would make distro packaging easier and be more consistent with other Zig projects.

@Techatrix
Copy link
Member Author

It would make distro packaging easier and be more consistent with other Zig projects.

I can agree with the "be more consistent with other Zig projects" part but this doesn't help downstream distributions. You don't need tracy to compile ZLS since it is completely optional.

@BratishkaErik
Copy link
Contributor

It would make distro packaging easier and be more consistent with other Zig projects.

I can agree with the "be more consistent with other Zig projects" part but this doesn't help downstream distributions. You don't need tracy to compile ZLS since it is completely optional.

In my case distro need to download it and unpack separately before compiling, or it will fail if user wants to enable it (simplified for not users of Gentoo):

# How it's now
TRACY_COMMIT="bla_bla" # zls-0.12.0.tar.gz from GitHub releases does not contain submodules, as tarball is auto-generated
# Downloading...

zig-build_src_unpack

rm -r src/tracy/
mv "../tracy-${TRACY_COMMIT}" src/tracy/
# ...
zig build ${package_flags} ${user_flags} --system "/bla_bla/"

@Techatrix
Copy link
Member Author

Why even package ZLS with tracy support?

@BratishkaErik
Copy link
Contributor

Why even package ZLS with tracy support?

If user wants to enable it, he will set up args and they will be passed like in code above. We don't know before building what args he chose, and at the same time want fully offline compilation, so along with ZLS source code we also have to download both tracy submodule and deps from build.zig.zon .

I'm just saying it would be nice to get rid of tracy submodule archive and just reuse second archive withh all deps from build.zig.zon .

@Techatrix
Copy link
Member Author

There is no reason for the users to enable tracy. It would be a waste of resources to even bother downloading it just in case.

@BratishkaErik
Copy link
Contributor

There is no reason for the users to enable tracy. It would be a waste of resources to even bother downloading it just in case.

Ok. Should I hardcode -Denable_tracy=false in user's compilation args then?

@Techatrix
Copy link
Member Author

Ok. Should I hardcode -Denable_tracy=false in user's compilation args then?

Yes, or just omit it if possible.

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

2 participants