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 track_path::path fn for usage in proc_macros #84029

Merged
merged 1 commit into from
Jul 2, 2021

Conversation

drahnr
Copy link
Contributor

@drahnr drahnr commented Apr 9, 2021

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 #73921

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 9, 2021
@rust-log-analyzer

This comment has been minimized.

@drahnr
Copy link
Contributor Author

drahnr commented Apr 9, 2021

Emits # file-dep:{} in the dependency file with the same escape logic as #71858 for the path.

I'd appreciate some bikeshedding on better naming in general, track_path could be misleading to be related to track_caller.

@drahnr drahnr marked this pull request as ready for review April 9, 2021 14:56
@drahnr drahnr changed the title add track! macro add track_path! macro Apr 9, 2021
@drahnr drahnr changed the title add track_path! macro add track_path! macro Apr 9, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

r? @petrochenkov

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@drahnr drahnr force-pushed the master branch 2 times, most recently from 9214959 to 11f646a Compare April 23, 2021 08:42
@petrochenkov
Copy link
Contributor

@drahnr
I understand the purpose of proc_macro::tracked_path::path, it is supposed to be used in proc macros to add dependencies on files.

But what is the purpose of the track_path! macro? Where is it supposed to be used?

  • Not in build scripts, they already have cargo:rerun-if-changed=my_file (https://doc.rust-lang.org/cargo/reference/build-scripts.html)
  • Not in proc macros, they already have proc_macro::tracked_path::path as mentioned above.
  • Not in declarative macros, all ways to access files from declarative macros are already tracked.

@petrochenkov
Copy link
Contributor

I think it's ok to add an interface like proc_macro::tracked_path::path that only adds a path to tracked files and does nothing else.

In #73921 (comment) I mentioned more high level interfaces for reading files as token streams (and simultaneously tracking them, of course).
However, the baseline tracking-only interface makes sense too, because it's always possible that your procedural macro will want to read files in some advanced ways (e.g. mmaping their parts into memory), which we cannot cover by higher level APIs like read_file_to_string or read_file_to_token_stream.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 25, 2021
@drahnr
Copy link
Contributor Author

drahnr commented Apr 26, 2021

Thanks for your feedback @petrochenkov !

I think it's ok to add an interface like proc_macro::tracked_path::path that only adds a path to tracked files and does nothing else.

In #73921 (comment) I mentioned more high level interfaces for reading files as token streams (and simultaneously tracking them, of course).
However, the baseline tracking-only interface makes sense too, because it's always possible that your procedural macro will want to read files in some advanced ways (e.g. mmaping their parts into memory), which we cannot cover by higher level APIs like read_file_to_string or read_file_to_token_stream.

Reasoning was mostly to get started with the rustc codebase and make sccache work for certain crates. In the sccache repo there was mention of another issue (I can't seem to find it right now) so I did not want to mix the concerns in one PR but rather have to distinct.

@drahnr
I understand the purpose of proc_macro::tracked_path::path, it is supposed to be used in proc macros to add dependencies on files.

But what is the purpose of the track_path! macro? Where is it supposed to be used?

* Not in build scripts, they already have `cargo:rerun-if-changed=my_file` (https://doc.rust-lang.org/cargo/reference/build-scripts.html)

* Not in proc macros, they already have `proc_macro::tracked_path::path` as mentioned above.

* Not in declarative macros, all ways to access files from declarative macros are already tracked.

I was taking the case into consideration where a shared crate is used, both in context of an regular application and a proc_macro. In the library context itself the shared code static libarary context, proc_macro::tracked_path::path would not be available iiuc, hence to allow this use case I suggest to add the builtin macro track_path.

Case example: https://github.com/djc/askama/blob/main/askama_shared/src/generator.rs#L796-L799

@drahnr
Copy link
Contributor Author

drahnr commented Apr 26, 2021

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 26, 2021
@petrochenkov petrochenkov added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 1, 2021
@rust-log-analyzer

This comment has been minimized.

@drahnr
Copy link
Contributor Author

drahnr commented Jul 1, 2021

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 1, 2021
@rust-log-analyzer

This comment has been minimized.

@Xanewok
Copy link
Member

Xanewok commented Jul 1, 2021

Diff in /checkout/compiler/rustc_interface/src/passes.rs at line 28:
 use rustc_plugin_impl as plugin;
 use rustc_query_impl::Queries as TcxQueries;
 use rustc_resolve::{Resolver, ResolverArenas};
+use rustc_serialize::json;
 use rustc_session::config::{CrateType, Input, OutputFilenames, OutputType, PpMode, PpSourceMode};
 use rustc_session::lint;
 use rustc_session::output::{filename_for_input, filename_for_metadata};
Diff in /checkout/compiler/rustc_interface/src/passes.rs at line 37:
 use rustc_span::FileName;
 use rustc_trait_selection::traits;
 use rustc_typeck as typeck;
-use tracing::{info, warn};
-use rustc_serialize::json;
 use tempfile::Builder as TempFileBuilder;
+use tracing::{info, warn};
 
 use std::any::Any;
 use std::cell::RefCell;
Diff in /checkout/compiler/rustc_interface/src/passes.rs at line 597:
         // Account for explicitly marked-to-track files
         // (e.g. accessed in proc macros).
         let file_depinfo = sess.parse_sess.file_depinfo.borrow();
-        let extra_tracked_files = file_depinfo
-            .iter()
-            .map(|path_sym| {
-                let path = PathBuf::from(&*path_sym.as_str());
-                let file = FileName::from(path);
-                escape_dep_filename(&file.prefer_local().to_string())
-            });
+        let extra_tracked_files = file_depinfo.iter().map(|path_sym| {
+            let path = PathBuf::from(&*path_sym.as_str());
+            let file = FileName::from(path);
+            escape_dep_filename(&file.prefer_local().to_string())
+        });
         files.extend(extra_tracked_files);
 
         if let Some(ref backend) = sess.opts.debugging_opts.codegen_backend {
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_target/src/abi/call/x86_win64.rs" "/checkout/compiler/rustc_interface/src/proc_macro_decls.rs" "/checkout/compiler/rustc_target/src/abi/call/msp430.rs" "/checkout/compiler/rustc_interface/src/passes.rs" "/checkout/compiler/rustc_target/src/abi/call/powerpc.rs" "/checkout/compiler/rustc_target/src/abi/call/sparc64.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.

Looks like it needs ./x.py fmt

@petrochenkov
Copy link
Contributor

I guess the moral of the story is that you really have to test locally, otherwise landing a PR turns into an attrition warfare.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 1, 2021
@drahnr
Copy link
Contributor Author

drahnr commented Jul 2, 2021

While I ran the tidy before the last set of nits, it's on me, failing to follow the guide to re-run the test + tidy checks.
Sorry again, I'll be better.

@drahnr
Copy link
Contributor Author

drahnr commented Jul 2, 2021

./x.py test as well as ./x.py test tidy --bless passed.

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 2, 2021
@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Jul 2, 2021

📌 Commit 67e6a81 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 2, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 2, 2021
Rollup of 7 pull requests

Successful merges:

 - rust-lang#84029 (add `track_path::path` fn for usage in `proc_macro`s)
 - rust-lang#85001 (Merge `sys_common::bytestring` back into `os_str_bytes`)
 - rust-lang#86308 (Docs: clarify that certain intrinsics are not unsafe)
 - rust-lang#86796 (Add a regression test for issue-70703)
 - rust-lang#86803 (Remove & from Command::args calls in documentation)
 - rust-lang#86807 (Fix double import in wasm thread )
 - rust-lang#86813 (Add a help message to `unused_doc_comments` lint)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 45470a3 into rust-lang:master Jul 2, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants