Refactor bounds definition. #325
Conversation
|
@Mark-Simulacrum Is this something you would be able to review? |
| /// date where the regression does not occur. | ||
| SearchNightlyBackwards { end: GitDate }, | ||
| /// Search between two commits. | ||
| Commits { start: String, end: String }, |
There was a problem hiding this comment.
IIUC, these aren't actually commits -- or at least indirectly so. E.g., origin/master is valid. Maybe we can note that in the comment?
There was a problem hiding this comment.
Good point! So there was only ever one case where it wasn't a SHA, and that was the origin/master case (when --end was not specified). This was later converted to a SHA. To keep things simple, I shifted the conversion earlier to when the Bounds is created so that the rest of the code can safely assume both values are a SHA.
The SHAs can still be shortened, and that can still be a source of bugs, but for the most part it works. That seems like something that could be further improved in the future to make it more robust.
| let date = args | ||
| .access | ||
| .repo() | ||
| .bound_to_date(Bound::Commit(tag.clone()))?; |
There was a problem hiding this comment.
This feels wrong to me. If we're looking up (say) 1.68.0, we presumably need something here to say that we want the stable release as of that date or otherwise go from the stable release to the nightly date range it "points" at?
Otherwise we're missing ~6 weeks here, right?
There was a problem hiding this comment.
The RustRepositoryAccessor::commit function returns the merge base with the master branch. So it essentially looks for the point where the given tag branched off the master branch. It does not return the commit associated with that tag.
That means it does not include any fixes merged to the beta branch, but should provide a close approximation of what the equivalent nightly was.
(This particular code is unchanged from the original, and has been working sufficiently well for me.)
There was a problem hiding this comment.
Ah, I missed that aspect. Sounds good to me.
src/bounds.rs
Outdated
|
|
||
| /// Returns the date of the latest nightly (fetched from the network). | ||
| fn find_latest_nightly() -> anyhow::Result<GitDate> { | ||
| let url = format!("{NIGHTLY_SERVER}/channel-rust-nightly.toml"); |
There was a problem hiding this comment.
https://static.rust-lang.org/dist/channel-rust-nightly-date.txt maybe works? Not sure if it's uploaded late enough in the process...
There was a problem hiding this comment.
Oh, that's better! I had forgotten about that.
From what I can tell, it looks like it should work, so I went ahead and switched to the txt file. From what I can see in promote-release, the last step it takes involves copying the manifests, including this txt file, and that happens after the archives have been uploaded.
There was only ever one case of it containing a non-hash, and that was when `--start` was a hash, and `--end` was not specified. In that case, it was "origin/master", and then converted to a hash later on. To simplify things, this converts `origin/master` to master's actual commit early when the `Bounds` is created.
This uses the text file to get the nightly date, which is faster and easier, and doesn't require toml parsing.
This is a refactoring of the bounds definitions in hopes to make the code easier to follow. I've struggled following it in the existing code because there are several mutations of
Opts::startandOpts::end, and the logic for interpreting the bounds was scattered in several places.A general overview of the change:
Boundsenum to represent the start and end boundaries. All code that accessed thestartandendvalues directly now looks atConfig::bounds.--startand--endhappens inBounds::from_argsin a single match.--by-commitand no--start(it now shows an error).This also fixes a bug I somehow run into several times. During the time between midnight UTC and whenever the nightlies are published, cbr will fail if you don't specify an
--end. This updates it to fetch the manifest from the network to determine the date of the latest nightly (find_latest_nightly) instead of assuming the current date. This requires pulling intomlto parse the manifest.Fixes #247