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

Icons are not initialized if CARGO_TARGET_DIR is used #12

Open
nazar-pc opened this issue Mar 14, 2024 · 23 comments
Open

Icons are not initialized if CARGO_TARGET_DIR is used #12

nazar-pc opened this issue Mar 14, 2024 · 23 comments

Comments

@nazar-pc
Copy link

The way config file is searched doesn't account for CARGO_TARGET_DIR and results in errors like this:

error: failed to run custom build command for `relm4-icons v0.8.2`

Caused by:
  process didn't exit successfully: `/home/nazar-pc/.cache/cargo/target/debug/build/relm4-icons-7ebbb2f82f7cb94e/build-script-build` (exit status: 101)
  --- stderr
  Canonical manifest dir: "/home/nazar-pc/.cache/cargo/target/debug/build/relm4-icons-9b161a11a467e328/out"
  thread 'main' panicked at /home/nazar-pc/.cargo/registry/src/index.crates.io-6f17d22bba15001f/relm4-icons-0.8.2/build.rs:68:17:
  Couldn't find your manifest directory
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

/home/nazar-pc/.cache/cargo/target is my target shared across multiple projects, I don't have target local to each crate.

@AaronErhardt
Copy link
Member

While this works great, this is still a somewhat hacked together solution because, after disallowing the use of many feature flags, Rust has no proper solution for communicating a lot of configuration options to build scripts. Hopefully, this gap will be filled with a new mechanism in the future.

I wasn't aware that global target dirs are a thing. The question is whether a build script is able to figure out the manifest directory of the root crate (the one which depends on relm4-icons) in this case. As I explained, I think there's no standardized solution for communicating such information to build scripts (or more precisely the only solution got cancelled by crates.io). The only available information are the env variables...

@nazar-pc
Copy link
Author

The question is whether a build script is able to figure out the manifest directory of the root crate (the one which depends on relm4-icons) in this case

I don't think there is and it is architecturally wrong in general.

Out of curiosity, what is the issue with including all icons? They are downloaded anyway and compiler should be able to eliminate unused constants (though this might require LTO, didn't check).

@AaronErhardt
Copy link
Member

Out of curiosity, what is the issue with including all icons? They are downloaded anyway and compiler should be able to eliminate unused constants (though this might require LTO, didn't check).

That's actually the same thing the crates.io team suggested when I asked them to remove the feature flag limitation. No, that doesn't work because GTK loads icons as bundled resources and the Rust compiler does not know what's in the resource bundle.

@nazar-pc
Copy link
Author

Well, then the alternative could be to expose a function that user of the library has to call from their build.rs rather than running it in this crate's build.rs.

@AaronErhardt
Copy link
Member

Well, then the alternative could be to expose a function that user of the library has to call from their build.rs rather than running it in this crate's build.rs.

I already thought about this earlier. We could definitely use macros for archiving the same result, albeit a bit less beautiful (AFAIK you can't tell Rust to rebuild code if files changed through a macro). I can try to get this to work.

@nazar-pc
Copy link
Author

AFAIK you can't tell Rust to rebuild code if files changed through a macro

Instructions to Rust (or Cargo?) are just printed to stdout, so should work. And any changes to build.rs trigger rebuilding by default, which is probably sufficient already.

@AaronErhardt
Copy link
Member

I think I've hit another dead end. If I build the bundle from this crate, it can't see the primary Cargo.toml without some hacks. If I build from the primary crate (e. g. by calling a fn in build.rs or by using a macro), I don't have access to the files of this crate which includes the shipped icons.

Overall, I think the best approach would be to keep this crate as it is for now and rather work on a proper solution that uses a different approach. E. g. relm4-template could be converted into an easily configurable template generator that automatically updates the generated files and supports multiple build systems (e. g. cargo, meson and flatpak). While doing this, we could also add a mechanism to download icons from here to the directory of the primary crate, making it possible to include them there with ease.

@nazar-pc
Copy link
Author

Overall, I think the best approach would be to keep this crate as it is for now and rather work on a proper solution that uses a different approach.

Well, I can't upgrade anymore because my development workflow is basically broken with new version.

E. g. by calling a fn in build.rs or by using a macro), I don't have access to the files of this crate which includes the shipped icons

Why not, what is the difficulty?

@AaronErhardt
Copy link
Member

AaronErhardt commented Mar 14, 2024

Well, I can't upgrade anymore because my development workflow is basically broken with new version.

That's how I felt when the crates.io team told me I can't publish a new stable version with the old mechanism that used feature flags. They broke it and didn't want to make an exception so I had to come up with a workaround. It's really unfortunate.

Why not, what is the difficulty?

AFAIK, you can only see the files in your current crate. You can't see the source code of your dependencies. So the build script or the macro wouldn't be able to access the icons shipped with this crate because they are executed by the primary crate.

I just had the idea that we could include every single icon file in the generated binary of the build library or the macro tough. That would be extremely ugly, but would be a robust solution.

@nazar-pc
Copy link
Author

So the build script or the macro wouldn't be able to access the icons shipped with this crate because they are executed by the primary crate.

Build script will still depend on relm4-icons (or build-specific sub-crate) that has its own compilation step. So it can compile some metadata that build.rs of the application can call and get the details necessary.

@AaronErhardt
Copy link
Member

Yeah, it should work like that.

The thing is though, because the release yesterday already took me more time than a whole work day with all the various steps included and I'm already short on free time, I'm not eager to work on this in the upcoming days. I hope you understand.

@nazar-pc
Copy link
Author

Sure, totally understand. I might take a stab at this myself depending on the mood.

@pieterdd
Copy link

pieterdd commented Mar 14, 2024

I get a similar error:

Caused by:
  process didn't exit successfully: `/home/pieter/Desktop/my-project/target/debug/build/relm4-icons-579924a03f1200f5/build-script-build` (exit status: 101)
  --- stderr
  Canonical manifest dir: "/home/pieter/Desktop/my-project/target/debug/build/relm4-icons-304ce4e06babc756/out"
  thread 'main' panicked at /home/pieter/.cargo/registry/src/index.crates.io-6f17d22bba15001f/relm4-icons-0.8.2/build.rs:75:36:
  called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish...

CARGO_TARGET_DIR, CARGO_MANIFEST_DIR and SOURCE_DIR are not set for me. My cargo config looks like this:

[target.x86_64-unknown-linux-gnu]
linker = "clang"
rustflags = ["-C", "link-arg=-fuse-ld=/usr/bin/mold"]

The error is triggered at this line, not this line like the OP's build did. Is it related to the same issue or is it best if I open a separate thread for this?

@hwittenborn
Copy link
Member

hwittenborn commented Jun 3, 2024

@nazar-pc @AaronErhardt: I'm not positive if I understood what was wrong, but it looks like you all are saying icons.toml can't be found when CARGO_TARGET_DIR is set.

If so, it looks like the PWD variable is set to the directory where cargo build is ran, irrespective of the project being built. That currently only works when you're in the directory containing Cargo.toml (and in turn icons.toml), but it should work irrespective of the CARGO_TARGET_DIR. There'd need to be more handling if you're in a subdirectory of the Cargo project, but at that point it's just a matter of walking up the directory tree until a Cargo.toml file is found.

There's a corner case where it wouldn't work when running cargo install, such as if you were to upload your GTK/Adwaita package to crates.io. Most GTK packages need to install other files (like app icons) onto the system though, so I'm not sure how much of an issue that is.

Let me know if I overlooked something you guys already looked into, but there's a PR open at #17 if you want to test things.

@nazar-pc
Copy link
Author

nazar-pc commented Jun 4, 2024

You shouldn't rely on PWD for anything (I'm not sure it is guaranteed to be set to what you expect it to be set to), in fact IIRC you can build project from one directory while being in a completely different directory. The only thing you can/should rely on in terms of paths is what Cargo provides you: https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-crates

Basically the whole current approach is broadly speaking flawed to begin with.

I think simply letting user pick icons with a call in their own build.rs as suggested above rather than trying to guess things is a much more robust approach.

It was not the highest priority for me for now, but if no one else does that, I will get to it at some point.

@hwittenborn
Copy link
Member

You're right about that, it does look like you can build outside of the project directory through the use of --manifest-path. And yeah, PWD isn't documented to be in any certain directory like you said.

I guess writing a new tool from scratch is going to be the best option. Cargo really does make this clunky to deal with :P

@hwittenborn
Copy link
Member

I think simply letting user pick icons with a call in their own build.rs as suggested above rather than trying to guess things is a much more robust approach.

Or that too, I think that'd be best honestly. Aaron probably has more insight than me on the best approach between the two though.

@AaronErhardt
Copy link
Member

I actually had an idea how to solve the issue entirely (not sure whether it works though). We could include each icon into its own resource bundle and only load it, if the constant of the icon name was used. This would actually allow dead-code elimination to remove the unused icons.

We could store each icon as SomeWrapper<Lazy<()>> that derefs as &'static str which represents the icon name and when the deref happens, it initializes its resource bundle if not done before.

@dastansam
Copy link

I started working on this, mostly based on @nazar-pc suggestions:

I think simply letting user pick icons with a call in their own build.rs as suggested above rather than trying to guess things is a much more robust approach.
and
Build script will still depend on relm4-icons (or build-specific sub-crate) that has its own compilation step. So it can compile some metadata that build.rs of the application can call and get the details necessary.

basically, I created a sub-crate relm4-icons-build that exposes a function to build icons for crate dependents. And as you pointed out, I am facing this issue with accessing shipped icons.

If I build from the primary crate (e. g. by calling a fn in build.rs or by using a macro), I don't have access to the files of this crate which includes the shipped icons.

could you tell more about this approach @AaronErhardt?

@AaronErhardt
Copy link
Member

If I build from the primary crate (e. g. by calling a fn in build.rs or by using a macro), I don't have access to the files of this crate which includes the shipped icons.

could you tell more about this approach @AaronErhardt?

The "approach" you've cited is actually a statement, so I don't know what I should answer. The general problem is always that you can't access files across crates, but only the resulting library. So if we compile all icons into a library, we could circumvent the problem. Yet, unless we have a mechanism for dead-code elimination, we add bloat to the final binary. Therefore, I suggested storing the icons individually bundled in static variables, that only get accessed (and initialized) if their icon name is derefed (Deref trait). This way, rustc should throw out all other statics from the resulting binary because they are not accessed.

@nazar-pc
Copy link
Author

And as you pointed out, I am facing this issue with accessing shipped icons

What is the issue exactly?

@dastansam
Copy link

And as you pointed out, I am facing this issue with accessing shipped icons

What is the issue exactly?

basically this one:

If I build from the primary crate (e. g. by calling a fn in build.rs or by using a macro), I don't have access to the files of this crate which includes the shipped icons.

I need the path to the icons shipped with this crate in the build script of the application

@nazar-pc
Copy link
Author

The reason I mentioned build crate is because build crate can have build.rs. In that build.rs you can trivially store CARGO_MANIFEST_DIR in a constant in generated file.

It will then be available either directly or through a specific function in build.rs of the crate that uses icons alongside with any other information you may need (like static list of all available icons, etc.). It can also do all kinds of processing of icons if necessary and so on.

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

No branches or pull requests

5 participants