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

[BUG] Modifying the file included with flate! doesn't automatically trigger rebuild #12

Closed
cyqsimon opened this issue Feb 9, 2023 · 11 comments · Fixed by #13
Closed
Labels
bug Something isn't working

Comments

@cyqsimon
Copy link
Contributor

cyqsimon commented Feb 9, 2023

To reproduce:

  1. Paste this code into main.rs:
fn main() {
    include_flate::flate!(static FOO_STR: str from "foo.bar");
    println!("{}", FOO_STR.as_str());
}
  1. Create a foo.bar under crate root, and write some content
  2. Run cargo run, and observe that the content of foo.bar is correctly printed
  3. Modify the contents of foo.bar and save
  4. Run cargo run again, and observe that a rebuild is not triggered, and the old contents of foo.bar are printed
  5. Replace flate! with include_str!, repeat steps 2-5, and observe that a rebuild is triggered

I am reporting this as a bug primarily because include-flate is advertised as a drop-in replacement for include_str, so this difference in behaviour could be confusing.

Also I agree with the standard library that automatically triggering a rebuild is the sensible default, so include-flate should try to replicate this behaviour. That being said, I'm not too familiar with the implementation details of include_str!, so perhaps it's driven by compiler magic and the automatic rebuild behaviour is not replicable in third-party crates, I'm not sure.

@SOF3 SOF3 added the bug Something isn't working label Feb 9, 2023
@SOF3
Copy link
Owner

SOF3 commented Feb 9, 2023

Build scripts typically solve this problem by printing cargo:rerun-if-changed=PATH on stdout. I am not sure if this approach works with proc macros.

@cyqsimon
Copy link
Contributor Author

I did a little research and came across those two issues:

These seem to be the exact features fixing this bug would need, so I guess we're waiting on those.

That being said, the author of the second issue did include his current workaround in his post. Maybe this is something we can do too in the meantime? I'll give it a try.

@cyqsimon
Copy link
Contributor Author

Yep, the workaround seems to work great. I tried it on one of my personal projects, and it seems to have the intended behaviour. You can try it too:

include-flate = {git = "https://github.com/cyqsimon/include-flate.git", branch = "auto-rebuild-hack", features = ["stable"]}

This shouldn't cause additional data to be included into the binary, but I tested it on one of my personal projects just in case:
image

Funnily the binaries got smaller but that's irrelevant. I'll make a PR for this.

@evolutics
Copy link

The workaround is fine for relative paths. But it seems to break builds when using absolute paths.

For example, given a path /home/me/project/asset.txt to include, the workaround resolves this to include a file /home/me/project//home/me/project/asset.txt, which fails.

The root cause of this seems to be include_str!(concat!(env!("CARGO_MANIFEST_DIR"), "/", $path)).

@cyqsimon
Copy link
Contributor Author

cyqsimon commented Apr 1, 2023

The workaround is fine for relative paths. But it seems to break builds when using absolute paths.

For example, given a path /home/me/project/asset.txt to include, the workaround resolves this to include a file /home/me/project//home/me/project/asset.txt, which fails.

The root cause of this seems to be include_str!(concat!(env!("CARGO_MANIFEST_DIR"), "/", $path)).

Interesting. I'll admit I did not consider absolute paths at all, although why would you want to do so? I'm curious.

@cyqsimon
Copy link
Contributor Author

cyqsimon commented Apr 1, 2023

After thinking about this for a bit on what can be done, I don't think there exists a perfect fix for this problem right now. It's not possible, as far as I know, to conditionally include a file without procedural macros; and the only way to track a file in procedural macros is with the unstable proc_macro::tracked_path::path. So we have two options, each with its benefits and drawbacks.

  1. Bite the bullet and use unstable API. This is the "proper" way to solve this, but will of course require nightly Rust. I can reuse the stable feature to ensure it's still possible to build with stable Rust, but it will be lacking the auto-rebuild trigger, as is the case before this issue.
  2. Split the flate macro into flate and flate_abs for now, and have the existing workaround in both. Then when proc_macro::tracked_path::path is stabalised (which may be a loooong way out), we can consider deprecating flate_abs.

@evolutics @SOF3 what do you think?

@SOF3
Copy link
Owner

SOF3 commented Apr 1, 2023

Is there an actual example scenario why you'd include an absolute path outside the workspace?

@cyqsimon
Copy link
Contributor Author

cyqsimon commented Apr 1, 2023

Is there an actual example scenario why you'd include an absolute path outside the workspace?

I struggle to think of one, although to be fair, there are provisions for this case in the proc macro:

let target = if path.is_relative() {
dir.join(path)
} else {
path
};

@evolutics
Copy link

Thanks for your ideas.

Just to be clear about my motivation: I don't need to use absolute paths. My use case is only a demo example of how to use another library in combination with include-flate, where relative paths work equally well. I just thought I'd let you know when I discovered this by accident.

Up to you to decide. Yet another alternative might be to simply not support absolute paths if people only need relative paths anyway.

@cyqsimon
Copy link
Contributor Author

cyqsimon commented Apr 3, 2023

@SOF3 what do you think I should do?

Perhaps completely remove support for absolute paths and explicitly state so in docs?

Or should I keep support and implement one of the above workarounds?

@SOF3
Copy link
Owner

SOF3 commented Apr 3, 2023

Personally I don't see any good reason for including absolute paths (other than using devfs or procfs, which is probably dangerous either way). I would opt for explicitly dropping support for absolute paths.

(Alright, maybe you want to use /etc/hostname, but you don't need to deflate that right?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants