Skip to content

pkg(highway): Enable system-integration #6668

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AnthonyZhOon
Copy link
Contributor

Move bridge.cpp to being a module cSourceFile instead of part of the staticlib Dynamic link against highway with pkg-config name 'libhwy.pc' when system-integrated

Some concerns:

  • Running ldd zig-out/bin/ghostty after building with zig build -Doptimize=ReleaseFast does not show libhwy.so but is included in debug optimize.

    • The staticlib variant also compiles without source files in release mode but not in debug due to missing symbols hwy::Abort().
    • Would look into how highway works more to understand this, it looks to extensively use templates and template-specialisations which results in little shared object code?
  • We configure a few optimization related flags (vectorization, constants), ran a quick DOOM-fire and cats and it seems okay

  • There's a few macro defines for predefined macros (DATE, TIMESTAMP, TIME) that are overrided in the static lib. The comment seems to imply that they are for reproducible binaries so I think it shouldn't change runtime behaviour.

@AnthonyZhOon
Copy link
Contributor Author

Checking highway's source, it simply appears that much of the implementation is in the header files and template instantiations. It should be fine that libhwy.so is entirely cleaned out by link-time optimizations.

@AnthonyZhOon AnthonyZhOon marked this pull request as ready for review March 14, 2025 01:34
@mitchellh
Copy link
Contributor

Please rebase for all the latest build changes, shouldn't be much work. Thank you!

@AnthonyZhOon
Copy link
Contributor Author

AnthonyZhOon commented Mar 15, 2025

I chose to add an import module in the SharedDeps.zig similar to how other libs that expose a zig module over C files now that bridge.cpp is not compiled into the static lib.
Although it seems the extern "c" fn hwy_supported_targets() i64 is not used in ghostty

@mitchellh mitchellh enabled auto-merge March 15, 2025 16:02
@mitchellh mitchellh disabled auto-merge March 15, 2025 16:02
@mitchellh
Copy link
Contributor

I'm going to merge this for now and I defaulted the system integration to true (for packaging). I am suspicious that this will work in practice because I've found each highway version changes the API in ways that its usually pretty tied but we'll see!

@mitchellh
Copy link
Contributor

I'm not at the computer but we need to add highway as a package to our Debian 12 test.

auto-merge was automatically disabled March 16, 2025 10:19

Head branch was pushed to by a user without write access

@AnthonyZhOon
Copy link
Contributor Author

Updated the Debian 12 build env

@mitchellh mitchellh enabled auto-merge March 16, 2025 13:47
@mitchellh
Copy link
Contributor

Okay yeah, this is what I expected. The dependency on Debian 12 is too old and the API isn't stable. I'm going to default the system integration to false and packagers who really care can enable it.

auto-merge was automatically disabled March 19, 2025 08:29

Head branch was pushed to by a user without write access

@AnthonyZhOon
Copy link
Contributor Author

Merged the Nix conflicts and built successfully with it

@mitchellh mitchellh enabled auto-merge March 19, 2025 15:44
@mitchellh mitchellh disabled auto-merge March 19, 2025 15:45
Copy link
Contributor

@mitchellh mitchellh left a comment

Choose a reason for hiding this comment

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

I think the merge with main was incorrect, it undoes some changes around the Nix files. I can take a look later but can't merge until then.

@AnthonyZhOon AnthonyZhOon requested review from a team as code owners March 24, 2025 14:48
@AnthonyZhOon AnthonyZhOon requested a review from a team as a code owner March 24, 2025 14:48
Copy link
Member

@hqnna hqnna left a comment

Choose a reason for hiding this comment

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

Please rebase your branch so it includes the po/nb_NO.UTF-8.po file, thanks.

Move bridge.cpp to being a module cSourceFile instead of part of the staticlib
Dynamic link against highway with pkg-config name 'libhwy.pc' when system-integrated
Expose hwy_supported_targets() from the highway zig module
@AnthonyZhOon
Copy link
Contributor Author

AnthonyZhOon commented Mar 24, 2025

I've taken the patch of the package changes and applied it onto main but left out the nix/ changes @mitchellh made

Copy link
Member

@hqnna hqnna left a comment

Choose a reason for hiding this comment

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

The changes look much cleaner now, and don't effect irrelevant files.

@AnthonyZhOon AnthonyZhOon requested a review from mitchellh March 27, 2025 13:47
Support `zig build -fsys=highway`
@AnthonyZhOon
Copy link
Contributor Author

Looking at the nix config, I've added libhwy to the devshell but not to the package.nix as we've decided not to have highway as a default system integration with zig build --system since glslang and spirv-cross integration are similarly not in the nix package.

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