Conan-center-index: [service] 2 PRs to the same library: new binaries on merge not generated

Created on 30 Sep 2019  路  12Comments  路  Source: conan-io/conan-center-index

If several Pull Requests are open for the same recipe, the packages belong to the recipe revision of the pull request, but after merging both, it will generate a third recipe revision with the merge of both changes and won't be binaries available for the latest.

The solution is to fire a new binary build in the merge but being careful, only if the recipe revision doesn't exist yet. Of course, anything could fail in the merge and we would need to fix/retry or whatever needed.

bug enhancement high

All 12 comments

Why not forcing to update from master before merging? It is a check we can activate in Github itself (I think we should activate _all_ protections for master in this repo)

It might be a good idea but I see a drawback we should consider: You move more workload to the contributor especially if the reviews/merges are not fast enough. If we could update the branches ourselves I won't have any doubt, but that is not possible if the users are not from the org, right? Is there a way to do that?

We can update it ourselves if they allow contributors to push changes to their PR branch (usually it is activated by default)

We have been talking and it is going to be a nightmare for the users, any change, in any library will outdate all the open PR. We already have 30 open, so imagine when we get traction.
We discussed different mechanisms and the proposal is:

  • When a PR is merged, we will check with GH API the open PRs. For each one, we will get the references affected (can be done checking the properties) and if some of them collide, we will do an automatic review to the PR to indicate the user has to update it because another improvement to the same recipe has been done. See *1

  • Also, I think we need to double-check after merging the PR that the resultant recipe revision is the same we are merging, otherwise (produced by merging 2 independent PRs) it will mean that we need to fire a new build for that package (maybe opening a PR with some dummy change or firing an internal build).

*1) If the mechanism in the first point works, the second point should not be necessary, like a PR queued with not binaries built yet. So, probably we need to mitigate this by calculating the affected refs first or trying to do the real diff of the contents doing the checkout of the PR (might be complex).

We can update it ourselves if they allow contributors to push changes to their PR branch (usually it is activated by default)

I don't think by default you can write in the branch of a contributor.

The documentation about that is here https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork although it doesn't say what is the default (allowed or not). One can deduce from this post in 2016 that the default is set to "Allow" https://github.blog/2016-09-07-improving-collaboration-with-forks/

I can say it has worked for me in the past

more workload to the contributor especially if the reviews/merges are not fast enough...

Github adds a special button to do that: "Update branch". It's just a clic and if nothing has changed affecting the recipe in the PR, the CI should detect the same revision and build nothing.

image

The problem is that the build will be fired, again and again, the system is based on the commit of the repository, not in the recipe revision, but maybe that is the mistake.

I think this is even more complicated:

  • pull request for recipe A is open
  • recipe A has a dependency on recipe B
  • recipe B is getting updated in master in the meantime, i.e. new recipe revision and new binaries for B exist
  • pull request for A is merged
  • outdated binaries for A would be published

So it is not only about the recipe A itself, but also every single dependency

In the worst case the update of B just broke A and only building from source would revel this

The downstream dependencies could be an issue in this situation and simply when you upgrade B, you should upgrade A, right? and recursively. But we want to go to a pure "all shared" or "all static" model and the headers for a third party in the same version shouldn't change. So this is mostly ok. We cannot (at least in this stage) afford to rebuild all the reverse dep tree in every library change.

Recipe revision is now checked when a PR is merged and the job fails if it is different from the revision of the PR branch. An internal notification is triggered indicating that we should manually regenerate the packages using the master branch recipe.

We'd improve this triggering new builds automatically in the future

This feature has been completed now, tested in staging env and deployed to production. Should be ready to work with the already open PRs of OpenSSL and Boost

Was this page helpful?
0 / 5 - 0 ratings