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 CI after Rust 1.83 #246

Merged
merged 5 commits into from
Dec 3, 2024
Merged

Fix CI after Rust 1.83 #246

merged 5 commits into from
Dec 3, 2024

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Dec 3, 2024

The path changes that @MarijnS95 outlined here seem to have been fixed in beta and in more recent nightlies, but did slip into the 1.83 release. So I've disabled doc tests on 1.83 in CI for now.

@madsmtm madsmtm added the bug Something isn't working label Dec 3, 2024
@madsmtm madsmtm requested a review from notgull December 3, 2024 16:09
Comment on lines 67 to 68
OPTIONS: ${{ matrix.platform.options }}
# Disable doc tests on Rust 1.83
OPTIONS: ${{ matrix.platform.options }} ${{ matrix.rust_version == 'stable' && '--lib' || '' }}
Copy link
Member

Choose a reason for hiding this comment

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

Shall we link an issue or comment describing this problem in more detail?

Copy link
Member Author

Choose a reason for hiding this comment

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

I linked to rust-lang/rust#132203

Copy link
Member

@MarijnS95 MarijnS95 Dec 3, 2024

Choose a reason for hiding this comment

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

Thank you for doing your due diligence to look up the actual upstream issue. I'll comment there later because it doesn't seem to be explicitly mentioned that this "regression" has trickled into 1.83 - i.e. they didn't fix / back it out before the release.

At the same time I hate to call this a regression, it's a good change. As outlined in #127 (comment) already, I feel like having the path relative to README.md (i.e. the span of that file) is much more sensible than having it relative to whichever random file (or files, it could be multiple in different locations) is/are including README.md.
But that requires a breaking change...

EDIT: upstream Rust issues and pull requests are incredibly poorly documented. They all reference each other, but none of them have an accurate summary describing what they're observing or changing, requiring one to follow links all the way to the beginning, then unwind again to understand what is going on:

  1. Span in rustdoc errors is incorrectly reported for include_str!(): rustdoc reports issues against lib.rs rather than an include_str!'d file rust-lang/rust#130470
  2. Fix for that: rustdoc: use the correct span for doctests rust-lang/rust#130582
  3. That fix regresses recursive imports: regression: change in paths/CWD for rustdoc tests rust-lang/rust#132203
  4. Fixing regression by making it an edition 2024 change: rustdoc: make doctest span tweak a 2024 edition change rust-lang/rust#132210
  5. Decision to change such behaviour must have a tracking issue etc: Tracking issue for Rust 2024: Fix doctest include paths rust-lang/rust#132230
    (Notice my frustration again: this spends more words explaining what a tracking issue is, than describing what it is tracking beyond "fixing the path used for include in rustdoc doctests")

Copy link
Member

@ids1024 ids1024 left a comment

Choose a reason for hiding this comment

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

Annoying that this issue got into Rust 1.83, but everything here looks good.

@madsmtm madsmtm merged commit fa3a918 into master Dec 3, 2024
38 checks passed
@madsmtm madsmtm deleted the madsmtm/fix-ci branch December 3, 2024 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

3 participants