cargo icon indicating copy to clipboard operation
cargo copied to clipboard

Bail publish job before packaging and upload (passing tests)

Open torhovland opened this issue 1 year ago • 11 comments

Builds on #13501 with a fix that makes the tests pass. Fixes #3662.

torhovland avatar Aug 01 '24 11:08 torhovland

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

rustbot avatar Aug 01 '24 11:08 rustbot

@hi-rustin, you recently refactored away the return tuple in fn cargo::ops::registry::registry(). Any thoughts about reintroducing a return tuple here?

torhovland avatar Aug 01 '24 11:08 torhovland

@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.

epage avatar Aug 01 '24 16:08 epage

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

stupendoussuperpowers avatar Aug 05 '24 19:08 stupendoussuperpowers

I think this approach, rather than set_quiet(true) makes more sense.

epage avatar Aug 05 '24 19:08 epage

stupendoussuperpowers:dupl

@torhovland - all tests are passing with this. you can apply this patch and we should be good to go.

stupendoussuperpowers avatar Aug 07 '24 06:08 stupendoussuperpowers

Good job!

@epage This is ready for another look.

@stupendoussuperpowers Will you follow up any feedback, please?

torhovland avatar Aug 07 '24 09:08 torhovland

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.

stupendoussuperpowers avatar Aug 07 '24 10:08 stupendoussuperpowers

For simplicity, feel free to reopen #13501 and finish the work there.

torhovland avatar Aug 07 '24 10:08 torhovland

@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.

0xPoe avatar Aug 07 '24 13:08 0xPoe

It'd be a big help for the commits to be cleaned up

Either

  1. Add tests with them passing
  2. Bail early
  3. Fix double-message

or

  1. Add tests with them passing
  2. Change return type of registry
  3. Bail early

epage avatar Aug 07 '24 13:08 epage

:umbrella: The latest upstream changes (presumably #14433) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Sep 06 '24 16:09 bors

#14448 was merged with these fixes. I believe we can close this.

stupendoussuperpowers avatar Sep 06 '24 20:09 stupendoussuperpowers