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(doctest): warn only once if doctest xcompile is skipped #14920

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

progressive-galib
Copy link

@progressive-galib progressive-galib commented Dec 11, 2024

as per issue/comment #12118 (comment) added warning about skipping doctest on xcompiling even when not under verbose flag

#this pr is an attempt at solving #12118. i tested them myself still might need a little work as concise waring shows every time we skip a test instead of once.

…ipping doctest on xcompiling even when not under verbose flag
@rustbot
Copy link
Collaborator

rustbot commented Dec 11, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added Command-test S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2024
@@ -194,6 +194,10 @@ fn run_doc_tests(
CompileKind::Host => {}
CompileKind::Target(target) => {
if target.short_name() != compilation.host {

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 the contribution.

We need a test for this warning to make sure it doesn't emit excessive warnings.

#12118 (comment)

Copy link
Author

@progressive-galib progressive-galib Dec 11, 2024

Choose a reason for hiding this comment

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

currently it will show every time a doctest is passed over (i.e skipped)

else we are gonna need to use something like,

pub static ref WARNONCE: Mutex<bool> = Mutex::new(false);

(btw i really like lazy_static crate but i kinda admire low package count of a self hosted compiler)

Copy link
Member

Choose a reason for hiding this comment

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

This part of cargo is single-threaded, so perhaps a simple variable here serves the purpose.

Copy link
Author

Choose a reason for hiding this comment

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

done, pushed it with without locally building.... man as someone working with very slow computer gotta say i just 💖 your CI/CD set up.

Copy link
Author

Choose a reason for hiding this comment

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

done, all tests have passed

@weihanglo weihanglo changed the title as per issue/comment https://github.com/rust-lang/cargo/issues/12118#… fix(doctest): warn only once if doctest xcompile is skipped Dec 11, 2024
@ehuss
Copy link
Contributor

ehuss commented Dec 12, 2024

I just want to mildly push back on this, since the original warning was hidden for a reason. It is difficult or impossible for the user to remove the warning. I suspect this will show up for everyone who runs cargo test --target which I suspect will be a large number. For example, I think it will start appearing in every CI run for rust-lang/rust. I understand the desire to bring awareness that something is being skipped, but if there is nothing the user can do about it, I'm not sure that is helpful. I'd like to better understand the motivation here.

@progressive-galib
Copy link
Author

I just want to mildly push back on this, since the original warning was hidden for a reason. It is difficult or impossible for the user to remove the warning. I suspect this will show up for everyone who runs cargo test --target which I suspect will be a large number. For example, I think it will start appearing in every CI run for rust-lang/rust. I understand the desire to bring awareness that something is being skipped, but if there is nothing the user can do about it, I'm not sure that is helpful. I'd like to better understand the motivation here.

•)it will only warn only once on the first doctest, not for each doctest,
•)its just one line in a 10000 line CI log, I agree with you better to know it was skipped for those that need them
•)if users need the warning turned off there can be a flag for once users use it and ask for the flag

@progressive-galib
Copy link
Author

@epage any update ?

@progressive-galib
Copy link
Author

@epage any update ?

sorry i meant @ehuss

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Command-test 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.

5 participants