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

Port to x11rb #40

Merged
merged 10 commits into from Jun 13, 2023
Merged

Port to x11rb #40

merged 10 commits into from Jun 13, 2023

Conversation

9ary
Copy link
Member

@9ary 9ary commented May 16, 2023

The long awaited.

This shouldn't be an overly difficult task, but it will need some care. One problem in particular is that xlib provides some utility functions that have no equivalent in XCB and x11rb, so they may need to be re-implemented in rust. Luckily, we can work on the port progressively thanks to the possibility to share the underlying XCB connection between x11rb and xlib.

One of the primary motivations for this port, besides switching to a vastly superior library, is the ability to build a completely self-contained static binary. Once the port is complete and no xlib code is left, we can switch over to the pure rust RustConnection instead of XCBConnection.

I've done some initial porting work. Because it's WIP, this PR is a draft. PRs against this branch are welcome. When the port is complete, we can do a final review here and then finally merge into master.

@9ary
Copy link
Member Author

9ary commented May 19, 2023

This branch is now complete. I wanted to improve error reporting, but I will leave that for another PR, this one is big enough as it is. Considering the size of the overall diff, consider reviewing each commit individually.

@9ary
Copy link
Member Author

9ary commented May 19, 2023

@jrmarino sorry for the unsolicited ping, but would you mind testing this PR? Two things:

  • I want to make sure I didn't break RGB565 support
  • you should now be able to cross-compile a static binary for your machine; after installing the right target with rustup, cargo build --release --target i586-unknown-linux-musl should hopefully give you something that works on that thing

@jrmarino
Copy link

Okay, I'll try to test both things

@jrmarino
Copy link

okay, building against musl target did not work. I tried 2 targets, i686-unknown-linux-musl and your suggested i586-unknown-linux-musl.

I got the same errors on both. Attached one build log. host was

Linux zaxxon 6.1.0-7-686 #1 SMP PREEMPT_DYNAMIC Debian 6.1.20-2 (2023-04-08) i686 GNU/Linux

musl-x11rb.log

@9ary
Copy link
Member Author

9ary commented May 19, 2023

It seems like you don't have the target installed. You need to install the toolchain with rustup and run rustup target add <target name>.

@jrmarino
Copy link

hmm, perhaps I deinstalled the target after I saw it didn't work

@jrmarino
Copy link

so I built a glibc-based no problem, but I am having problems testing it. Both the binary from this build, and the previous one that worked are failing to open the display. Since the previous one worked before, it's something with the xorg server. I had an MIT cookie issue I solved before with the "xhost +local:" command, but I had to run that from the terminal, and I think this is something else. The machine must have been rebooted since the last time I checked it.

in the meantime, I'll try the musl build again.

@9ary
Copy link
Member Author

9ary commented May 19, 2023

Interesting, I hope this isn't a bug in x11rb, but since you're running into the same issue with your known-good xlib build we're probably good.

@jrmarino
Copy link

no, it's not shotgun's issue. I can't even run xpdyinfo. can't find the display

@jrmarino
Copy link

I installed both i586 and i686 targets:

marino@zaxxon:~/shotgun$ rustup target list | grep musl
aarch64-unknown-linux-musl
arm-unknown-linux-musleabi
arm-unknown-linux-musleabihf
armv5te-unknown-linux-musleabi
armv7-unknown-linux-musleabi
armv7-unknown-linux-musleabihf
i586-unknown-linux-musl (installed)
i686-unknown-linux-musl (installed)
mips-unknown-linux-musl
mips64-unknown-linux-muslabi64
mips64el-unknown-linux-muslabi64
mipsel-unknown-linux-musl
x86_64-unknown-linux-musl

Neither worked. Log for i586 attached.
musl-i586.log

@9ary
Copy link
Member Author

9ary commented May 19, 2023

The log you attached makes no mention of musl or i586, but I can see gnu, glibc and i686 in there.

For what it's worth I do intend to provide prebuilt binaries via CI with the next release, but for now you could try this build: shotgun-i586-unknown-linux-musl.zip

@jrmarino
Copy link

did i build it incorrectly? I used the command you suggested, "cargo build --release --target i586-unknown-linux-musl"

@jrmarino
Copy link

I got a colleague to run shotgun via VNC and the latest xllrb version worked fine, no regression.
I have yet to download the musl version and test it though. I will have to try the vnc route I guess.

@psychon
Copy link

psychon commented May 19, 2023

Nice PR. Out of curiosity, I took a look at things and have nothing to comment.

Out of more curiosity, I wonder whether you could comment on the size of shutgun with and without this PR. Is it much larger? (Why do I expect things to be larger: Because x11rb has some actual parsing code and no shared library. Xlib has parsing code, but that is in libX11.so and not in the final binary. See also psychon/x11rb#488 .)

src/xwrap.rs Outdated Show resolved Hide resolved
@9ary
Copy link
Member Author

9ary commented May 19, 2023

Nice PR. Out of curiosity, I took a look at things and have nothing to comment.

Thanks for taking a look!

Out of more curiosity, I wonder whether you could comment on the size of shutgun with and without this PR. Is it much larger? (Why do I expect things to be larger: Because x11rb has some actual parsing code and no shared library. Xlib has parsing code, but that is in libX11.so and not in the final binary. See also psychon/x11rb#488 .)

Good point and great question.

Test environments:

  • Laptop: Arch Linux, up to date, rustc 1.69.0 from rustup
  • Desktop: NixOS 22.11, rustc 1.64.0

Building in release mode, and then stripping the binary.

Target Laptop (master) Laptop (x11rb) Desktop (master) Desktop (x11rb)
x86_64-unknown-linux-gnu 611k 783k 623k 787k
i686-unknown-linux-gnu 670k 858k
x86_64-unknown-linux-musl 883k
i586-unknown-linux-musl 846k
i686-unknown-linux-musl 850k

All in all no big surprise. It's probably possible to link against libx11 statically for more comparison points, but I'm not in the mood to set this up right now. I also wasn't able to build a 32 bit glibc/xlib version on Arch for some reason, and cross-compilation on NixOS seems a little more complicated, at least without using rustup.

Laptop, x86_64-unknown-linux-gnu:

  • master:
$ cargo bloat --release
   Compiling shotgun v2.4.0 (/home/streetwalrus/source/shotgun)
    Finished release [optimized] target(s) in 1.05s
    Analyzing target/release/shotgun

File  .text     Size       Crate Name
0.5%   5.6%  21.9KiB         std addr2line::ResDwarf<R>::parse
0.4%   4.8%  18.7KiB         std std::backtrace_rs::symbolize::gimli::resolve::{{closure}}
0.2%   2.7%  10.7KiB         std addr2line::ResUnit<R>::parse_lines
0.2%   2.2%   8.6KiB miniz_oxide miniz_oxide::inflate::core::decompress
0.2%   2.1%   8.1KiB miniz_oxide miniz_oxide::inflate::core::decompress
0.1%   1.7%   6.6KiB     getopts getopts::Options::parse
0.1%   1.4%   5.7KiB         std gimli::read::abbrev::Abbreviations::insert
0.1%   1.4%   5.6KiB         std gimli::read::unit::parse_attribute
0.1%   1.4%   5.6KiB miniz_oxide miniz_oxide::deflate::core::compress_block
0.1%   1.4%   5.6KiB     shotgun shotgun::run
0.1%   1.4%   5.6KiB miniz_oxide miniz_oxide::deflate::core::compress_inner
0.1%   1.1%   4.2KiB         png png::filter::filter_internal
0.1%   1.0%   3.8KiB         png png::encoder::Writer<W>::write_image_data
0.1%   1.0%   3.8KiB         png png::encoder::Writer<W>::write_image_data
0.1%   1.0%   3.7KiB         std addr2line::function::Function<R>::parse_children
0.1%   0.9%   3.7KiB         std std::backtrace_rs::symbolize::gimli::Context::new
0.1%   0.9%   3.5KiB         std core::slice::sort::recurse
0.1%   0.9%   3.5KiB miniz_oxide miniz_oxide::deflate::core::HuffmanOxide::optimize_table
0.1%   0.9%   3.5KiB         std <core::iter::adapters::map::Map<I,F> as core::iter::traits::iterator::Iterator>::next
0.1%   0.8%   3.1KiB         std std::backtrace_rs::symbolize::gimli::elf::<impl std::backtrace_rs::symbolize::gimli::Mapping>::new_debug
5.5%  63.4% 248.5KiB             And 1056 smaller methods. Use -n N to show more.
8.6% 100.0% 391.9KiB             .text section size, the file size is 4.4MiB
  • x11rb:
$ cargo bloat --release
   Compiling shotgun v2.4.0 (/home/streetwalrus/source/shotgun)
    Finished release [optimized] target(s) in 1.59s
    Analyzing target/release/shotgun

 File  .text     Size          Crate Name
 0.5%   4.3%  21.9KiB            std addr2line::ResDwarf<R>::parse
 0.4%   3.7%  18.7KiB            std std::backtrace_rs::symbolize::gimli::resolve::{{closure}}
 0.2%   2.1%  10.7KiB            std addr2line::ResUnit<R>::parse_lines
 0.2%   1.7%   8.6KiB    miniz_oxide miniz_oxide::inflate::core::decompress
 0.2%   1.6%   8.1KiB    miniz_oxide miniz_oxide::inflate::core::decompress
 0.1%   1.3%   6.9KiB        getopts getopts::Options::parse
 0.1%   1.2%   6.1KiB        shotgun shotgun::run
 0.1%   1.2%   6.0KiB          x11rb x11rb::rust_connection::RustConnection::connect
 0.1%   1.1%   5.8KiB x11rb_protocol x11rb_protocol::protocol::request_name
 0.1%   1.1%   5.7KiB            std gimli::read::abbrev::Abbreviations::insert
 0.1%   1.1%   5.6KiB            std gimli::read::unit::parse_attribute
 0.1%   1.1%   5.6KiB    miniz_oxide miniz_oxide::deflate::core::compress_block
 0.1%   1.1%   5.6KiB    miniz_oxide miniz_oxide::deflate::core::compress_inner
 0.1%   0.8%   4.2KiB            png png::filter::filter_internal
 0.1%   0.7%   3.8KiB            png png::encoder::Writer<W>::write_image_data
 0.1%   0.7%   3.8KiB            png png::encoder::Writer<W>::write_image_data
 0.1%   0.7%   3.7KiB            std addr2line::function::Function<R>::parse_children
 0.1%   0.7%   3.7KiB            std std::backtrace_rs::symbolize::gimli::Context::new
 0.1%   0.7%   3.5KiB            std core::slice::sort::recurse
 0.1%   0.7%   3.5KiB    miniz_oxide miniz_oxide::deflate::core::HuffmanOxide::optimize_table
 7.4%  70.0% 355.9KiB                And 1477 smaller methods. Use -n N to show more.
10.6% 100.0% 508.4KiB                .text section size, the file size is 4.7MiB

9ary added 3 commits May 21, 2023 15:59
As a first step, this initializes the connection and replaces xlib types
with x11rb ones in the API of xwrap.
Copy link
Member

@Lonami Lonami left a comment

Choose a reason for hiding this comment

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

Nice. Perhaps a few things would make more sense as const.

src/util.rs Show resolved Hide resolved
src/util.rs Outdated Show resolved Hide resolved
src/xwrap.rs Show resolved Hide resolved
src/xwrap.rs Outdated Show resolved Hide resolved
src/xwrap.rs Show resolved Hide resolved
src/xwrap.rs Show resolved Hide resolved
src/xwrap.rs Outdated Show resolved Hide resolved
src/xwrap.rs Show resolved Hide resolved
src/xwrap.rs Outdated Show resolved Hide resolved
@9ary
Copy link
Member Author

9ary commented Jun 13, 2023

🚀

@9ary 9ary merged commit e7cdd55 into master Jun 13, 2023
2 checks passed
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

4 participants