8000 Fix file includes by Malax · Pull Request #506 · heroku/libcnb.rs · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix file includes #506

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

Merged
merged 3 commits into from
Sep 29, 2022
Merged

Fix file includes #506

merged 3 commits into from
Sep 29, 2022

Conversation

Malax
Copy link
Member
@Malax Malax commented Sep 29, 2022

Cargo cannot include files outside of a package's root directory. We currently have includes that violate this rule. This never surfaced since we don't depend on these files being included and Cargo doesn't warn when a file couldn't be included. This isn't well documented either. (refs: https://doc.rust-lang.org/cargo/reference/manifest.html#the-exclude-and-include-fields, rust-lang/cargo#10617 (comment))

The workaround here is using symlinks which will be packaged up correctly. I tested this via cargo package, both using --list as well as inspecting the resulting .crate file.

In addition, the README file for libcnb is located outside of the package root directory as well. Since we include that file as documentation in the source, compilation fails when the packaged crate is being compiled from scratch. This prevents vendoring (via cargo vendor) of libcnb.

I'm not entirely sure why the README include doesn't work with cargo vendor since the README.md is correctly included in the .crate file I manually downloaded from crates.io. I suspect that the Cargo metadata that references the README file outside of the package root directory makes cargo vendor skip this file. This also means I cannot 💯 confirm this PR fixes the vendoring issue without publishing to crates.io.

Closes GUS-W-11828824

@Malax Malax marked this pull request as ready for review September 29, 2022 08:45
@Malax Malax requested a review from a team as a code owner September 29, 2022 08:45
Copy link
Member
@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Thank you!

Sounds like this would be a useful thing to report upstream too :-) (The lack of a warning)

@edmorley
Copy link
Member

Sounds like this would be a useful thing to report upstream too :-) (The lack of a warning)

Ah there is already: rust-lang/cargo#9667

@edmorley
Copy link
Member

Also: rust-lang/cargo#5911

@Malax Malax enabled auto-merge (squash) September 29, 2022 09:33
@Malax Malax merged commit 3e7fb83 into main Sep 29, 2022
@Malax Malax deleted the malax/fix-file-includes branch September 29, 2022 09:34
@Malax
Copy link
Member Author
Malax commented Sep 29, 2022

Version 0.11.1 has been released and the vendoring issue is indeed fixed now. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0