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

Add a macro to track a file #73921

Closed
GuillaumeGomez opened this issue Jul 1, 2020 · 19 comments
Closed

Add a macro to track a file #73921

GuillaumeGomez opened this issue Jul 1, 2020 · 19 comments
Labels
A-proc-macros Area: Procedural macros C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@GuillaumeGomez
Copy link
Member

I recently released doc-comment 0.4 which converted declarative macros to proc-macros. I encountered a "funny" issue though: I needed the compiler to track files that were included using my proc-macros. In order to do so, I generated anonymous constants which looked like this:

const _: &'static str = include_str!("file.md");

However, I have no use for those constants and they make the source code heavier for just keeping track of a file. Therefore, I propose to add a track!("file") macro which would allow to track files like include_str! without requiring the usage of anonymous constants.

I can make the implementation if the rust teams agree to add this feature. :)

@GuillaumeGomez GuillaumeGomez added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 1, 2020
@GuillaumeGomez GuillaumeGomez self-assigned this Jul 1, 2020
@GuillaumeGomez GuillaumeGomez changed the title Add a macro to track a file last modification time Add a macro to track a file Jul 1, 2020
@Mark-Simulacrum
Copy link
Member

IMO, the proper solution is to add proc_macro APIs that indicate "interest" in a file, rather than further expanding the set of macros.

@estebank
Copy link
Contributor

estebank commented Jul 1, 2020

Isn't this already supported using build.rs?

@ehuss
Copy link
Contributor

ehuss commented Jul 3, 2020

The user of the proc-macro would need to add it to their build.rs, which is a bit tedious (and I suspect most people won't even know they should do that). It would also cause the entire build script to rerun, which could be quite expensive if it is doing other unrelated things. Build scripts can also be tricky for inexperienced users (such as requiring "rerun-if-changed=build.rs" to avoid full rebuilds).

I've seen it mentioned in several places where there is desire to register change detection for various things (including environment variables mentioned here).

Grepping through crates.io, I found about 10 proc-macros that use the include_str/include_bytes trick to force a dependency check.

@ehuss ehuss added the A-proc-macros Area: Procedural macros label Aug 3, 2020
@ehuss
Copy link
Contributor

ehuss commented Aug 3, 2020

cc #74690, where the ability to register the usage of an environment variable was added. I would suspect adding support for tracking files could use a similar mechanism. I'm not sure if this specifically needs to use a macro, or if a function call would be sufficient (tracked_env::file(...)? tracked_env::path(...)?).

@petrochenkov
Copy link
Contributor

The bare minimum would be ability to add file paths to dependencies.

However, it would be significantly more useful to also have ability to load the file into a token stream and create a source map for it, so all the spans are assigned properly.

@luser
Copy link
Contributor

luser commented Sep 29, 2020

However, it would be significantly more useful to also have ability to load the file into a token stream and create a source map for it, so all the spans are assigned properly.

This is #55904

@jonas-schievink jonas-schievink added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Apr 9, 2021
drahnr added a commit to drahnr/rust that referenced this issue Jul 2, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jul 2, 2021
add `track_path::path` fn for usage in `proc_macro`s

Adds a way to declare a dependency on external files without including them, to either re-trigger the build of a file as well as covering the use case of including dependencies within the `rustc` invocation, such that tools like `sccache`/`cachepot` are able to handle references to external files which are not included.

Ref rust-lang#73921
@abonander
Copy link
Contributor

The documentation for proc_macro::tracked_path::path() says:

Track a file explicitly.

However, I just tested it in the process of fixing launchbadge/sqlx#681 and it seems to work for directories too. Is this intentional? Because if it's not, I would love for it to continue working as-is because it'll let us finally fix that bug.

And if it is intentional or it's decided that working for directories is a feature and not a bug, it should be mentioned in the documentation.

@abonander
Copy link
Contributor

abonander commented Jul 20, 2021

It's actually kind of an issue for us that path() takes impl AsRef<str> as we need to use Path::canonicalize() to get a path that this API can accept (I assume, see next paragraph), so we then need to do the fallible conversion back to &str which is a lot of boilerplate to use this function.

On that note, the function should probably document how it treats relative paths, if at all. Is it relative to the workspace root? If that's the case then we don't even need to canonicalize.

@petrochenkov
Copy link
Contributor

it seems to work for directories too. Is this intentional?

The specified file path gets into the depinfo file written by rustc.
How it's treated further depends entirely on tools interpreting that depinfo.

IIRC, cargo recently supported directories cargo:rerun-if-changed=PATH (https://doc.rust-lang.org/cargo/reference/build-scripts.html#cargorerun-if-changedpath), so directories in depinfo may be interpreted in the same way, but it needs to be clarified cargo people.

@petrochenkov
Copy link
Contributor

petrochenkov commented Jul 20, 2021

It's actually kind of an issue for us that path() takes impl AsRef<str>

+1, it should probably accept a &Path, if it's possible to pass through proc macro bridge (#84029 (comment)).

@ehuss
Copy link
Contributor

ehuss commented Jul 20, 2021

Yea, this will be dependent on the build system. I think for Cargo it is safe to rely on that behavior, as it is documented in the link given.

Taking a &Path probably sounds like the right thing to do, but it will likely break in horrible ways if using non-utf8 paths, so it may give a false sense of support.

@luser
Copy link
Contributor

luser commented Jul 20, 2021

Taking a &Path probably sounds like the right thing to do, but it will likely break in horrible ways if using non-utf8 paths, so it may give a false sense of support.

This is a sensible position, and is exactly why @sunshowers created camino. It's annoying, but perhaps the API should take AsRef<Path>, and be fallible so it can reject non -UTF-8 paths? I'm not sure if that would make things more usable or just move the pain elsewhere.

Taking a Path would also allow sidestepping the concerns @abonander raised, in that the implementation could be responsible for serializing the path in the necessary way (canonicalized, absolute, crate-relative, whatever).

@sunshowers
Copy link
Contributor

yeah, depinfo is an instance of what I call the "makefile problem". Given that rustc uses these files, should it support non-utf8 paths at all? cargo already doesn't.

I guess you don't need cross-platform support for depinfo files, so the case is a bit less strong for rustc than for cargo.

@abonander
Copy link
Contributor

There's a couple orthogonal concerns here:

The API really needs to document what it does with relative paths. If it's purely up to whatever is consuming the depinfo then that should be documented as well, though it would also be very helpful to describe how Cargo treats it in that case. SQLx's proc macros are already written with the assumption that Cargo is being used, although that has also drawn a few complaints.

If we don't want to specify the behavior with relative paths and just tell the user to always canonicalize the path, that's okay. We just need to say one way or the other.

As for non-UTF-8 paths, I'm torn. In SQLx's case the paths are taken from string literals in the macro input so they should always be UTF-8.

If I knew I didn't need to manipulate the paths at all, I'd be perfectly fine with the function taking impl AsRef<str>.

If the treatment of relative dirs was specified but differed from my expectation, say for example, I'm expecting the paths to be relative to the crate root whereas Cargo interprets the paths relative to the workspace root, then I would still need to do path manipulation and impl AsRef<Path> would be nicer.

For sanity if we take impl AsRef<Path> then the function should be fallible and return an error on non-UTF-8 paths. That's just the sanest way to do it and lets the proc macro keep control over what to do next. However, that's an unnecessary annoyance if you're always dealing with paths as &strs.

Do we have anything against having two different functions? Say, path_str(impl AsRef<str>) and path(impl AsRef<Path>), where the former is infallible?

@pksunkara
Copy link
Contributor

Tangential question: What's the path forward to stabilize this? I don't see any other PRs which are tracking this feature.

@drahnr
Copy link
Contributor

drahnr commented Aug 11, 2021

@pksunkara the PR #87173 is probably the next step.

@abonander Adding two versions seems excessive compared to the feature provided, #87173 (comment) resolves this by a steering meeting discussion outcome, to use a Path based argument in order to not limit the API to the limitations of today's implemention.

I am not aware of any further issues at this point.

@abonander
Copy link
Contributor

abonander commented Aug 12, 2021

@drahnr please see my concerns above about specifying the semantics regarding relative paths and directories, that doesn't appear to have been discussed at all in the linked PR.

@drahnr
Copy link
Contributor

drahnr commented Aug 12, 2021

The API really needs to document what it does with relative paths. If it's purely up to whatever is consuming the depinfo then that should be documented as well, though it would also be very helpful to describe how Cargo treats it in that case. SQLx's proc macros are already written with the assumption that Cargo is being used, although that has also drawn a few complaints.

If we don't want to specify the behavior with relative paths and just tell the user to always canonicalize the path, that's okay. We just need to say one way or the other.

@abonander indeed, I'll document the behaviour on relative paths and which base will be used in detail.

@m-ou-se
Copy link
Member

m-ou-se commented Jul 20, 2022

This is now tracked in #99515, together with the tracked_env feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-proc-macros Area: Procedural macros C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests