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

fix: cargo docs breaking in buildDepsOnly of workspaces #410

Merged
merged 3 commits into from
Oct 4, 2023

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Oct 2, 2023

Motivation

Fix #312 by using a different name for dummy binary in every package (derived from the package name itself). This should make cargo doc happy. I tested lightly and it seems to work.

Checklist

  • added tests to verify new behavior
  • added an example template or updated an existing one
  • updated docs/API.md (or general documentation) with changes
  • updated CHANGELOG.md

@ipetkov
Copy link
Owner

ipetkov commented Oct 2, 2023

Thanks for the PR! Is it possible to add a test for this to avoid future regressions?

@dpc
Copy link
Contributor Author

dpc commented Oct 2, 2023

Now that I have some idea how the tests work, I can try adding it later today.

BTw. I don't understand the test failed

@ipetkov
Copy link
Owner

ipetkov commented Oct 2, 2023

We have some tests that cleanCargoToml outputs things as expected. Since this changes the name used for dummy binaries those tests might need to be updated as well (I'd recommend building the failing test locally to help with debugging)

@dpc dpc force-pushed the cargo-doc-deps-only-fix branch from 5082d14 to 7f10f3f Compare October 3, 2023 17:58
@dpc
Copy link
Contributor Author

dpc commented Oct 3, 2023

Fixed, added test, made sure test failed before, rebased, pushed. @ipetkov

Copy link
Owner

@ipetkov ipetkov left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test, LGTM!

@ipetkov ipetkov merged commit 0da36bc into ipetkov:master Oct 4, 2023
8 checks passed
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.

Duplicate crane-dummy after upgrading crane to v0.12.1
2 participants