cargo
cargo copied to clipboard
Bail publish job before packaging and upload (passing tests)
Builds on #13501 with a fix that makes the tests pass. Fixes #3662.
r? @weihanglo
rustbot has assigned @weihanglo. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
@hi-rustin, you recently refactored away the return tuple in fn cargo::ops::registry::registry(). Any thoughts about reintroducing a return tuple here?
@hi-rustin, you recently refactored away the return tuple in
fn cargo::ops::registry::registry(). Any thoughts about reintroducing a return tuple here?
The goal with that refactor is less about removing the tuple and more about pulling work out of that function for reuse, like @jneem's recent transmit -> prepare_transmit + transmit refactor. Adding back in a tuple to access some of the other state is reasonable.
As for the 2 failing tests -
credential_process::token_caching - This is failing because the test case tries to upload a package twice to test out two different scenarios. However it does it with the same version number and therefore our new code changes mean that the second test publish job bails. I have verified a fix for this locally where we just need to change the version and expected stdout.
registry_auth::token_not_logged - Here, we are logging the token 8 times instead of the previous 7. I wonder if this is because of the extra check we make to confirm whether the crate@version exists already? If so, we can either change the test to expect 8 logs (if that is the new accepted behaviour), or check why there's an extra log.
Let me know how I should proceed if we are okay with continuing with this solution.
CC: @torhovland
I think this approach, rather than set_quiet(true) makes more sense.
@torhovland - all tests are passing with this. you can apply this patch and we should be good to go.
Good job!
@epage This is ready for another look.
@stupendoussuperpowers Will you follow up any feedback, please?
I'm assuming I'll have to rebase to be 2 commits - first with the test and second with the updates. Will get to it.
For simplicity, feel free to reopen #13501 and finish the work there.
@hi-rustin, you recently refactored away the return tuple in
fn cargo::ops::registry::registry(). Any thoughts about reintroducing a return tuple here?
I think you are attempting to return a different type. What I removed from it is RegistrySourceIds, but what you actually want to return here is RegistrySource. So, I believe it is okay to add it.
It'd be a big help for the commits to be cleaned up
Either
- Add tests with them passing
- Bail early
- Fix double-message
or
- Add tests with them passing
- Change return type of
registry - Bail early
:umbrella: The latest upstream changes (presumably #14433) made this pull request unmergeable. Please resolve the merge conflicts.
#14448 was merged with these fixes. I believe we can close this.