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

Move icon toggling to a build script #25

Merged
merged 11 commits into from
Oct 29, 2023
Merged

Move icon toggling to a build script #25

merged 11 commits into from
Oct 29, 2023

Conversation

carloskiki
Copy link
Owner

No description provided.

add path dependency to libs
added build_rs to templates

add anyhow to build deps
commit icon list file
workign build scripts with env variable
Update Readme

add reference to index
set a icondata manifest for tests

add cargo config for icondata.toml

correctly set msrv

clippy
@carloskiki carloskiki merged commit 711a04f into main Oct 29, 2023
10 checks passed
@lpotthast
Copy link
Contributor

Hi, could you sum up the implications of these changes? I also looked into fixing the feature problem today. Havent noticed the discussion two weeks ago. We really screwed crates.io with the amount of features 🙈..
The problem seems to be around the enums. Removing them allows proper dead code elimination. I thought that the same benefit, naming an icon without using the data struct, having something to serialize, etc., would also be possible by defining a unit struct per icon which could provide a &'static IconData. This would allow us to remove ALL features alltogether and users would also not have to name the icons they are using anymore. What do you think?

@carloskiki carloskiki deleted the build-script branch October 29, 2023 12:39
@lpotthast
Copy link
Contributor

Ahh.. This would only work when dropping serialization support, as even a custom deserializer would bring in all known icon variants through a match statement... Might be somehing to reconsider.

@carloskiki
Copy link
Owner Author

Hi, could you sum up the implications of these changes? I also looked into fixing the feature problem today. Havent noticed the discussion two weeks ago. We really screwed crates.io with the amount of features 🙈..

Oh yes we did 😅

The problem seems to be around the enums. Removing them allows proper dead code elimination. I thought that the same benefit, naming an icon without using the data struct, having something to serialize, etc., would also be possible by defining a unit struct per icon which could provide a &'static IconData. This would allow us to remove ALL features alltogether and users would also not have to name the icons they are using anymore. What do you think?

Dead Code elimination already works with enum when using lto = true or lto = "fat" (from my testing). At first I wanted to simply remove all of the feature flags and constrain people to use lto as a "hot-fix" while we find a better solution. The only reason I decided to hack a build script together in the mean time is that importing every icon by default completely destroys rust-analyser as it pollutes the namespace.

I didn't think of the implications of serde and I think your solution is a better than mine for that reason. I haven't tested DCE with unit structs, but if it does work then I would be happy to implement your solution, but users would have to use the qualified syntax e.g., icondata::AiMyIcon if they want their IDE to work.

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