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 1774585 - Collect required files to perform a build #1187

Closed

Conversation

lissyx
Copy link
Contributor

@lissyx lissyx commented Jun 16, 2022

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2022

Codecov Report

Merging #1187 (67b1bb7) into main (c6bc422) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1187      +/-   ##
==========================================
- Coverage   29.22%   29.16%   -0.06%     
==========================================
  Files          47       47              
  Lines       16764    16819      +55     
  Branches     8020     8058      +38     
==========================================
+ Hits         4899     4905       +6     
- Misses       6785     6839      +54     
+ Partials     5080     5075       -5     
Impacted Files Coverage Δ
src/compiler/rust.rs 31.88% <0.00%> (-1.30%) ⬇️
src/lib.rs 12.52% <0.00%> (-0.03%) ⬇️
src/compiler/gcc.rs 57.39% <0.00%> (ø)
src/azure/blobstore.rs 21.18% <0.00%> (ø)
src/compiler/msvc.rs 43.69% <0.00%> (+0.14%) ⬆️
src/lru_disk_cache/mod.rs 41.03% <0.00%> (+0.30%) ⬆️
src/mock_command.rs 51.68% <0.00%> (+0.37%) ⬆️
src/util.rs 35.35% <0.00%> (+0.63%) ⬆️
src/dist/cache.rs 21.05% <0.00%> (+0.75%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6bc422...67b1bb7. Read the comment docs.

@glandium
Copy link
Collaborator

I'm not going to have the time to investigate this in a timely manner, but invoking cargo is likely to cause problems in corner cases, so this should be investigated very thoroughly before it can be merged.

@lissyx
Copy link
Contributor Author

lissyx commented Jun 17, 2022

I'm not going to have the time to investigate this in a timely manner, but invoking cargo is likely to cause problems in corner cases, so this should be investigated very thoroughly before it can be merged.

If there's a better solution to that, I'm open, but I needed a quick fix ...

@lissyx lissyx force-pushed the bug1774585_collect_package_list_deps branch from 67b1bb7 to 3056a7b Compare June 17, 2022 00:10
@glandium
Copy link
Collaborator

Random additional thought: the underlying issue you're working around here is is not only a problem for distributed compilation. There's also a problem for caching, whereby building after changes to a file read by a proc-macro can have a cache hit from a build without changes.

@lissyx
Copy link
Contributor Author

lissyx commented Jun 17, 2022

Random additional thought: the underlying issue you're working around here is is not only a problem for distributed compilation. There's also a problem for caching, whereby building after changes to a file read by a proc-macro can have a cache hit from a build without changes.

You mean that build system should be aware that changes to e.g. Types.kt from here https://searchfox.org/mozilla-central/rev/d3c2f51d89c3ca008ff0cb5a057e77ccd973443e/third_party/rust/uniffi_bindgen/src/bindings/kotlin/gen_kotlin/mod.rs#100 are not going to be taken into account?

Should not that be fixed on the crate side, e.g., with a build.rs that has https://doc.rust-lang.org/cargo/reference/build-scripts.html#rerun-if-changed against those templates ?

@drahnr
Copy link
Collaborator

drahnr commented Jun 20, 2022

To track files as dependencies at proc-macro execution is a solution to this issue rust-lang/rust#84029 - in the past include_bytes! in the library being compiled/using the proc-macro was a solution as well, at the cost of including some binary data in the resulting binary.

@lissyx lissyx closed this Jun 22, 2022
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

Successfully merging this pull request may close these issues.

4 participants