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 small bug with rustflags and add more CI to test very basic cases #10

Closed
wants to merge 7 commits into from

Conversation

jonaspleyer
Copy link

This PR fixes some code duplication (see f7d3df9) and introduces multiple basic layouts to test various common examples of workspaces and custom rustdoc flags.

The cargo_config_rustflags example was failing for me before removing the duplicate code.

I am very happy about feedback.

PS: I have been using your libraries (like many people do) extensively and I very much appreciate the work that you do for the community.

Copy link
Owner

@dtolnay dtolnay 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 the PR. I don't think this is the right fix — docs.rs does pass --config build.rustflags, for example in this build: https://docs.rs/crate/proc-macro2/1.0.79/builds/1154848.

@jonaspleyer
Copy link
Author

Thanks for the PR. I don't think this is the right fix — docs.rs does pass --config build.rustflags, for example in this build: https://docs.rs/crate/proc-macro2/1.0.79/builds/1154848.

But there is some code duplication in lines 189 to 193 in main.rs which does not seem correct to me. The build you sent should still pass when applying my patch from commit f7d3df9.

@dtolnay
Copy link
Owner

dtolnay commented Apr 7, 2024

One of those is configuring build.rustflags and the other is configuring host.rustflags. They are not duplicated.

@jonaspleyer
Copy link
Author

This is clearly an oversight on my part and I see now why my initial attempt of fixing is wrong. I will probably come back to this. Are you still interested in the additional test cases?

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Feel free to open a new PR if you come back to this. I don't think I need these tests in this repo, but I am interested in fixes if you identify any bug.

@dtolnay dtolnay closed this May 19, 2024
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.

2 participants