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

Try to reduce the dependency graph #321

Closed
elinorbgr opened this issue May 21, 2020 · 19 comments
Closed

Try to reduce the dependency graph #321

elinorbgr opened this issue May 21, 2020 · 19 comments

Comments

@elinorbgr
Copy link
Member

elinorbgr commented May 21, 2020

the dependency graph of wayland-rs is relatively large, which seems a little big for a low-level crate like this. We probably can try to reduce it.

graph

@kchibisov
Copy link
Member

You don't need regex for xcursor-rs crate at all, so it could be all wiped if you're fine taking similar impl from someone under wayland-rs upstream. if you're fine I can send patches.

@jplatte
Copy link
Contributor

jplatte commented May 22, 2020

If regex is removed, it should be easy to get rid of lazy_static too, by replacing it with once_cell which wayland-commons already depends on.

@kchibisov
Copy link
Member

kchibisov commented May 23, 2020

Basically one can be replaced with another, right, since after regex removal both should be just leafs on this graph afaics, so it's a matter of preference, what suits more, I guess?

@kchibisov
Copy link
Member

regex was removed.

@elinorbgr
Copy link
Member Author

Yes, now the graph looks like that:

graph

One I'd like to figure out how to remove would be parking_lot, which currently provides a ReentrantMutex that wayland-server uses to ensure that the ffi to libwayland-server.so is never accessed from two different threads conccurently, as the lib as a whole is basically !Sync.

@kchibisov
Copy link
Member

Why winapi is on that graph?

@elinorbgr
Copy link
Member Author

Mostly because cargo deps is not very good at filtering platform-specific dependencies, I think.

@kchibisov
Copy link
Member

kchibisov commented May 27, 2020

well, we see that nom is a big problem here, maybe one could rewrite xcursor parsing without it :- )

@jplatte
Copy link
Contributor

jplatte commented May 27, 2020

well, we see that nom is a big problem here, maybe one could rewrite xcursor parsing without it :- )

What about this instead? esposm03/xcursor-rs#6 :)

@kchibisov
Copy link
Member

How much it reduces @jplatte ?

@jplatte
Copy link
Contributor

jplatte commented May 27, 2020

That removes the dependency on lexical-core, which means the dependencies on cfg-if, ryu, static-assertions, arrayvec and rustc_version also disappear as xcursor deps (though wayland-rs will continue to depend on cfg-if).

@kchibisov
Copy link
Member

The mentioned PR was merged and xcursor version was bumped, so graph could be updated.

@jplatte
Copy link
Contributor

jplatte commented May 28, 2020

By playing with the command line flags until I had this: cargo deps --no-transitive-deps --subgraph wayland-test wayland-protocols wayland-egl wayland-server wayland-client wayland-cursor wayland-scanner wayland-commons wayland-sys --subgraph-name wayland-rs | dot -Tpng > graph.png

I got the following updated graph:

graph

@jplatte
Copy link
Contributor

jplatte commented May 28, 2020

By excluding wayland-scanner and its dependencies (I'm guessing it's not flagged as a build dependency because it's part of the workspace) and target-specific features for platforms that wayland doesn't run on (winapi, cloudabi, redox_syscall, wasi), the remaining dependencies that actually contribute to the output size are:

graph

Of which I think

Can't say much about the other ones, but I think there won't be much more to get rid of :)

@kchibisov
Copy link
Member

tempfile shouldn't be on a graph as well, I guess, since it was excluded manually from previous graphs, since it's a build time dependency.

@jplatte
Copy link
Contributor

jplatte commented May 28, 2020

graph

@elinorbgr
Copy link
Member Author

tempfile and byteorder are actual dev-dependencies of wayland-client, and are only used in the examples, so these can be excluded. cargo deps does not yet handle correctly dev-deps in workspaces.

@jplatte
Copy link
Contributor

jplatte commented May 28, 2020

Oh, okay, that's what going wrong :)

So all that's left to do it potentially find a replacement for parking_lot, and replacing lazy_static with once_cell? Sounds like this issue could be closed in favor of two new issues that track those specific todos, right?

@elinorbgr
Copy link
Member Author

Well, parking-lot will be difficult to get rid of by itself, but will probably disappear when I implement the ideas from #317. I'm not sure if there is much to gain by getting rid of lazy_static, as it is a quite pervasive crate, hence there is a high probability that it already appears somewhere else in the dependency graph anyway. So I think we can call it a day for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants