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

Fingerprint out of root targets #15205

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

krtab
Copy link
Contributor

@krtab krtab commented Feb 19, 2025

This PR partially fixes #15203 and #12266 but this is admitedly a bit hackish, let me know how you want to proceed with it.

@rustbot
Copy link
Collaborator

rustbot commented Feb 19, 2025

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 19, 2025
@krtab
Copy link
Contributor Author

krtab commented Feb 19, 2025

This is the hack described in #12266 (comment) and hence has the same caveats that #[path] attributes won't be supported.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for helping with this issue!

As you've said, this is hacky. Some high-level feedback:

  • PathSource::list_file (which then calls _list_files) is used in computing fingerprints for build scripts and cargo doc, as well as determining what is going to be packaged into cargo package. During packaging, any file outside the package root will not be packed into the resulting .crate file. Cargo even emits a warning when the situation is detected. With this change, cargo-package starts including those files again. This is undesired, not even mention that where should their path be in the .crate tarball.
  • How do this hack know what really needs to track? It simply looks at the directory the target source file is in and assume it always at the root directory with all other relevant files. Yeah it cannot handle #[path] we already knew, but it also doesn't respect git directory and include/exclude options. It may accidentally include more files they necessary.

To me I still think depinfo from rustdoc is a more sustainable approach. We might want to put more effort on that route than sorting out a hack.

Some other alternatives are on user side, like breaking them into sub packages for reasonable reuse.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! It is linked in #12266 (comment), which also calls out a relevant Cargo issue #9931.

@weihanglo
Copy link
Member

Also note that the issue is not labeled as S-accepted. It is recommended to discuss designs in issues rather than in pull requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incremental cargo doc doesn't support [lib] path to parent
4 participants