Refactor build script and expose build functions for crate dependents #19

dastansam · 2024-08-02T13:23:02Z

This PR tries to address the issue #12 and also do a little bit of refactoring of build script:

this crate's build script was refactored to save and pass the crate's manifest path as a path to shipped icons
function is exposed under build-utils feature flag for crate dependents to bundle icons and save icon names in the constant.
docs updated

nazar-pc

I expected the whole icons.toml to be replaced with either a section in Cargo.toml or direct code when calling build script. A separate file in the root of the project is annoying.

nazar-pc · 2024-08-07T06:59:22Z

src/lib.rs

-pub fn initialize_icons() {
-    gio::resources_register_include!("resources.gresource").unwrap();
+#[macro_export]
+macro_rules! initialize_icons {


Why does this need to be a macro? Looks like a function should work just like before. If you really want to make sure compiler removes it when not necessary add #[inline] to the function definition.

gio::resources_register_include!("resources.gresource") tries to load the gresource but since I am moving the resource bundling to the application's build script from this crate, it won't find the resources.gresource in the OUT_DIR of this crate

Then specify an absolute path there instead of relative

gio::resources_register_include! tries to read OUT_DIR env variable which is not set in the relm4-icons crate because it doesn't have build script anymore. Or am I missing sth?

All the macro does is calling a hidden function internally. You do not need either macro or that function strictly speaking, just copy these 3 lines and call them with whatever arguments you may need:
https://github.com/gtk-rs/gtk-rs-core/blob/e1ad60d1c8b38634f638f50ec2b36cdc62462daf/gio/src/resource.rs#L35-L37

converted it back to function, copied those 3 lines. but it's loading the gresource in runtime now. is there other way to to make it compile time and include it in the binary?

Yes, to call include_bytes! macro just like above code does to store contents of a file in a constant

You provide contents of the file as an argument rather than its path in that case

Cargo.toml

src/build_utils.rs

nazar-pc · 2024-08-15T13:53:46Z

src/lib.rs

-pub fn initialize_icons() {
-    gio::resources_register_include!("resources.gresource").unwrap();
+#[macro_export]
+macro_rules! initialize_icons {


Then specify an absolute path there instead of relative

build_icons/README.md

build_icons/src/lib.rs

build_icons/README.md

AaronErhardt

From my side there are no additional objections that would prevent merging. Once @nazar-pc is satisfied as well, I suggest we merge the PR and publish a new alpha release that can be tested and gradually improved.

Also thanks for the continuous work on this PR!

README.md

build_icons/build.rs

nazar-pc

Almost there, also please test this if you can, I think if you did you'd see that it didn't actually work properly yet.

nazar-pc · 2024-08-29T01:03:38Z

src/lib.rs

+    let display = gdk::Display::default().unwrap();
+    let theme = gtk::IconTheme::for_display(&display);
+    theme.add_resource_path("/org/gtkrs/icons/");
+    theme.add_resource_path("/org/gtkrs/icons/scalable/actions/");


This does not look like an equivalent change. It only previously applied if app_id was not specified, but now it applies unconditionally despite the fact that app ID is required.

Looking at generation code, I think you'll want to store prefix in a constant and then call initialize_icons with it. Maybe @AaronErhardt can clarify, but I think it should not be necessary to have both resource paths, just the prefix used in generation code should be sufficient.

Yeah, the code in question is more a hack than an actual documented feature of GTK. It is not strictly harmful to call it unconditionally, but if the user has specified an app ID, the path of the icons can be adjusted such that it doesn't need this hack in the first place (that's why the if was there). We might as well deprecate or remove this code path entirely instad of relying on it.

nazar-pc · 2024-08-29T01:12:46Z

build_icons/build.rs


    // Create file that contains the icon names as constants
    let constants = format!(
        "
        /// Path to the shipped icons.
        pub const SHIPPED_ICONS_PATH: &str = \"{}/icons\";",
-        icons_path.display()
+        manifest_path


This will work, but consider proper path handling, including when it has ":

std::fs::write( std::path::Path::new(&out_dir).join(CONSTANTS_FILE), manifest_path.join("icons').canonicalize().to_str().unwrap(), ) .unwrap();

Storing OsString would be even more valid, but it requires platform-specific code to decode back on read, this way it will simply fail on non-utf8 characters at build time.

nazar-pc · 2024-10-10T02:23:00Z

@dastansam one more push, please 🙏

nazar-pc

@AaronErhardt this is almost ready, please take another look when you have a chance

nazar-pc · 2024-10-14T22:54:37Z

build_icons/build.rs

+        pub const SHIPPED_ICONS_PATH: &str = \"{}\";",
+        std::path::Path::new(&manifest_path)
+            .join("icons")
+            .canonicalize()
+            .unwrap()
+            .to_str()
+            .unwrap()


I'll post the same code snippet again:

std::fs::write( std::path::Path::new(&out_dir).join(CONSTANTS_FILE), manifest_path.join("icons').canonicalize().to_str().unwrap(), ) .unwrap();

Note that it stored the path in the file, not in the string inside of the file. Why? Because your version will result in corrupted file if path contains ".

src/lib.rs

…te-build-script

nazar-pc

Looks fine to me assuming it works

AaronErhardt · 2024-10-25T10:03:14Z

Thanks for all the work! Let's test the code and work towards a new stable release :)

dastansam force-pushed the refactor-crate-build-script branch 2 times, most recently from 32cf9f1 to 168626c Compare August 2, 2024 13:53

dastansam changed the title ~~Extract build specific logic to build crate, refactor crate~~ Refactor build script Aug 2, 2024

dastansam changed the title ~~Refactor build script~~ Refactor build script and expose build functions for crate dependents Aug 2, 2024

Extract build specific logic to build crate, refactor crate

07c9fb1

dastansam force-pushed the refactor-crate-build-script branch from 168626c to 07c9fb1 Compare August 2, 2024 15:05

Fix CI

b175bdd

dastansam marked this pull request as ready for review August 5, 2024 13:59

dastansam mentioned this pull request Aug 6, 2024

Add icons build script autonomys/space-acres#265

Closed

nazar-pc reviewed Aug 7, 2024

View reviewed changes

Resolve comments

99c0c18

dastansam requested a review from nazar-pc August 8, 2024 21:51

nazar-pc reviewed Aug 15, 2024

View reviewed changes

selfhoster1312 mentioned this pull request Aug 18, 2024

cargo clippy panic #20

Open

AaronErhardt reviewed Aug 19, 2024

View reviewed changes

build_icons/README.md Outdated Show resolved Hide resolved

Remove Config and make constants file configurable

f24bb69

dastansam requested review from nazar-pc and AaronErhardt August 26, 2024 16:09

AaronErhardt approved these changes Aug 26, 2024

View reviewed changes

Convert initialize_icons to function

133b001

dastansam force-pushed the refactor-crate-build-script branch from 91adb0e to 133b001 Compare August 27, 2024 12:55

nazar-pc reviewed Aug 27, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

build_icons/build.rs Outdated Show resolved Hide resolved

dastansam added 2 commits August 28, 2024 15:47

Move icons to build_icons

7d33e62

Update icons path

95bc7de

dastansam requested a review from nazar-pc August 28, 2024 13:52

nazar-pc reviewed Aug 29, 2024

View reviewed changes

Add resource prefix constant

bfbd358

dastansam requested a review from nazar-pc October 13, 2024 13:12

nazar-pc reviewed Oct 14, 2024

View reviewed changes

Merge branch 'main' of github-dastansam:Relm4/icons into refactor-cra…

4120f43

…te-build-script

dastansam force-pushed the refactor-crate-build-script branch from 2d136c0 to 4120f43 Compare October 15, 2024 08:36

dastansam requested a review from nazar-pc October 15, 2024 08:36

nazar-pc approved these changes Oct 15, 2024

View reviewed changes

AaronErhardt merged commit c86eda6 into Relm4:main Oct 25, 2024
5 checks passed

nazar-pc mentioned this pull request Oct 25, 2024

Simplify API #24

Merged

nazar-pc mentioned this pull request Dec 4, 2024

Icons are not initialized if CARGO_TARGET_DIR is used #12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor build script and expose build functions for crate dependents #19

Refactor build script and expose build functions for crate dependents #19

dastansam commented Aug 2, 2024 •

edited

Loading

nazar-pc left a comment

nazar-pc Aug 7, 2024

dastansam Aug 8, 2024

nazar-pc Aug 15, 2024

dastansam Aug 26, 2024

nazar-pc Aug 26, 2024

dastansam Aug 27, 2024

nazar-pc Aug 27, 2024

nazar-pc Aug 27, 2024

nazar-pc Aug 15, 2024

AaronErhardt left a comment

nazar-pc left a comment

nazar-pc Aug 29, 2024

AaronErhardt Aug 29, 2024

nazar-pc Aug 29, 2024

nazar-pc commented Oct 10, 2024

nazar-pc left a comment

nazar-pc Oct 14, 2024

nazar-pc left a comment

AaronErhardt commented Oct 25, 2024

Refactor build script and expose build functions for crate dependents #19

Refactor build script and expose build functions for crate dependents #19

Conversation

dastansam commented Aug 2, 2024 • edited Loading

nazar-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronErhardt left a comment

Choose a reason for hiding this comment

nazar-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc commented Oct 10, 2024

nazar-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc left a comment

Choose a reason for hiding this comment

AaronErhardt commented Oct 25, 2024

dastansam commented Aug 2, 2024 •

edited

Loading