Remote index branch configuration

[PaulDance]

Dear all,

It is my first time contributing to this great project, so some things may not be the best. This is an attempt to close #7329, which still seemed desirable and which I thought was relatively simple, but there were definitely some challenges to it.

A few notes about the implementation:

• I didn't have many ideas for how to name the feature, so I chose alternative-branches, which is always shorter than "alternative registries index alternative branches". Better naming is welcome.
• ops::RegistryConfig has a new branch field which is retrieved from the configuration using Config::get_registry_branch with the known registry name.
• The value is then passed onto sources::registry::RegistrySource::remote and saved in the already-existing index_git_ref field of sources::registry::remote::RemoteRegistry.
• RegistrySource::remote is also used by core::source::source_id::SourceId::load, but the name field is not always set, so Config::get_registry_branch_from_id is used in order to try getting it back by finding which registry configuration has the same index URL. This is actually a source of bug when packaging and therefore publishing: if two registries are configured with the same URL but different branches, the heuristic will yield inconsistent results between different executions, so some dependency may be correctly found once but not during the next run in identical conditions. I'm not very happy with this, but I really don't known how to get the registry name otherwise. I would very much appreciate some guidance here to get this to work consistently.
• The feature is hidden behind an unstable CLI flag of the same name.
• Basic documentation in the unstable section has been added.
• I have implemented dedicated test support in order to facilitate the addition of tests specific to this feature. It is mostly inspired by what is already available for regular alternative registries:
  • Add a way to setup some branches with git::RepoBuilder.
  • Add an alternative_branch field and its builder methods to registry::RegistryBuilder and registry::Package so we may correctly manipulate repositories and fake the publication accordingly.
  • alt_br_init: helper function to do the same as alt_init but with an additional alternative-branch branch.
• Tests have been added in alt_registry as I thought they would naturally belong there, but maybe it would warrant another module. They were mostly done by taking tests for regular registries and adapting them to check if they still work with or without using the alternative branch.

Remarks and suggestions are obviously welcome. I hope this could prove useful to some.

Cheers,
Paul.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[bors]

☔ The latest upstream changes (presumably #9475) made this pull request unmergeable. Please resolve the merge conflicts.

[PaulDance]

r? @ehuss
This should probably be considered a draft actually.

[ehuss]

Hi, thanks for the PR! And sorry for the slow response.

Just some high-level feedback before reviewing the code itself, I think the branch should be a part of the SourceId itself. Can you think of any particular problems with that? That is, the branch would be encoded in the SourceId in a fashion similar to how it is done with a git dependency (like registry+https://example.com/index?branch=foo). That should help simplify some things and ensure the correct branch is encoded in Cargo.lock and published Cargo.toml files.

As for bikeshedding the unstable name, if you want another option I might suggest registry-branch. But I don't have any preference, and I think it's fine to keep what you have.

[PaulDance]

Thanks for the feedback @ehuss!

Indeed, putting the branch information in SourceId itself as well is a good idea because it would enable storing it in Cargo.lock files and most important de-serialize correctly from them too. Correct me if I'm wrong, but I think that last bit was the blocker for me: at least when packaging, Cargo first reads the configuration which enables accessing the registry's alternative branch as the registry's name is known, does its thing to produce Cargo.lock and then only looks at that in order to do the rest which does not require the configured registry's name. In any case, thanks for pointing me towards this part of the operation I did not know about.

I took a good look at the code for SourceId and I don't really see any major problems with that strategy. I think adding a GitReference field to SourceKind::Registry would be a nice way of implementing the change, which is similar to what is done for SourceKing::Git. The uses of SourceKind::Registry throughout the module all seem to be adaptable to a defined behavior compatible with most situations. I won't have time to try this tonight but I think this week-end I will, I'll let you know if it is indeed realistic then.

As for the unstable option name, yes, it is a little bit very much bikeshedding, but I think your suggestion registry-branches is better for the option's name itself, while alternative-branch still seems adequate for test fields and values. I'll have to see how it fits.

[bors]

☔ The latest upstream changes (presumably #9657) made this pull request unmergeable. Please resolve the merge conflicts.

[ehuss]

Ping @PaulDance Just checking in to see if you are still interested in working on this, or if you had any questions.

[PaulDance]

@ehuss Yeah sorry, I had a bit of trouble making things work at the time and since then, I did not take the time to try again. It's not that I forgot about it though. I would like to finish it, but I have to motivate myself again first.

[wt]

@PaulDance I merged with the lastest upstream at [email protected]:wt/cargo.git on the branch remote-index-branch. Would you be willing to pull it in to resolve the conflict above?

Upstream folks, what would it take to get this to a mergeable state?

[PaulDance]

@wt Alright, I'll try to take a look at it tomorrow.

[wt]

FWIW, I pushed the wrong commit before. I think I fixed it now. I force pushed the branch, so if you pulled before, you'll need to pull it again.

[PaulDance]

@wt I have just now pushed everything I had on my side plus a merge.

Notes:

• I had some commits I did not push because I was working on refactoring things at the time, so I found it simpler to do the merge again myself.
• There is one last test that fails. I wasn't able to make it work at the time and even though I took a look again right now, I had the same difficulty to find the root cause.

As things stand currently, I don't think I will have the energy necessary in order to fix the issue all by myself, although it may be quite simple in practice. Can anyone investigate the test failure? It would help me quite a bit.

[wt]

I'll see if I can find time. This would be amazing to get working.

…

On Tue, Feb 15, 2022 at 2:21 PM Paul Mabileau ***@***.***> wrote: @wt <https://github.com/wt> I have just now pushed everything I had on my side plus a merge. Notes: - I had some commits I did not push because I was working on refactoring things at the time, so I found it simpler to do the merge again myself. - There is one last test that fails. I wasn't able to make it work at the time and even though I took a look again right now, I had the same difficulty to find the root cause. As things stand currently, I don't think I will have the energy necessary in order to fix the issue all by myself, although it may be quite simple in practice. Can anyone investigate the test failure? It would help me quite a bit. — Reply to this email directly, view it on GitHub <#9466 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADFT7YCFPU7MHPUE3PALWTU3LGXJANCNFSM44LA4C3Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- You're More Amazing Than You Think! ymatyt.com

[bors]

☔ The latest upstream changes (presumably #10470) made this pull request unmergeable. Please resolve the merge conflicts.

[ehuss]

I'm closing this due to inactivity. If you (or anyone else) wants to pick this up again, feel free to open a new PR.
Thanks for your contribution!