scheduler: loosen ordering for upgrade jobs

Currently the scheduler splits pretty much every upgrade job in
practice. This is because when both the old and new packages for A
depend on the old and new packages for B respectively, the strict rules
of the scheduler see a cycle and don't allow upgrading B while A is
install to avoid a state where old A is installed but old B is not.

I believe this rule to be too strict when applied to upgrade jobs and
have loosened the "old depends on old" rule to ignore upgrade jobs where
the "new depends on new" rule also applies. This patch passes all
existing test cases, including the test cases checking that split
upgrades are done when necessary. Therefore, I do not think the new
rules are too loose.

This patch also adds a basic test case that was not passed with the old
logic, I'm very surprised we didn't have a test checking this.

Sponsored by:	The FreeBSD Foundation

Author: ifreund

libpkg/pkg_jobs_schedule.c

@@ -95,6 +95,11 @@ enum pkg_jobs_schedule_graph_edge_type {
  * 3. A's old package conflicts with B's new package
  * 4. A and B are the two halves of a split upgrade job
  *    and A is the delete half.
+ *
+ * There is one exception made to rule 2 in order to avoid splitting in
+ * the common case of upgrading packages X and Y where Xold depends on Yold
+ * and Xnew depends on Ynew. In this case, rule 2 is ignored and there is no
+ * edge from X to Y.
  */
 static enum pkg_jobs_schedule_graph_edge_type
 pkg_jobs_schedule_graph_edge(struct pkg_solved *a, struct pkg_solved *b)
@@ -160,17 +165,22 @@ pkg_jobs_schedule_graph_edge(struct pkg_solved *a, struct pkg_solved *b)
 	if (a_new != NULL && b_new != NULL &&
 	    pkg_jobs_schedule_direct_depends(b_new, a_new)) {
 		return (PKG_SCHEDULE_EDGE_NEW_DEP_NEW);
-	} else if (a_old != NULL && b_old != NULL &&
-		   pkg_jobs_schedule_direct_depends(a_old, b_old)) {
-		return (PKG_SCHEDULE_EDGE_OLD_DEP_OLD);
-	} else if (a_old != NULL && b_new != NULL) {
+	}
+	if (a_old != NULL && b_new != NULL) {
 		struct pkg_conflict *conflict = NULL;
 		while (pkg_conflicts(a_old, &conflict) == EPKG_OK) {
 			if (STREQ(b_new->uid, conflict->uid)) {
 				return (PKG_SCHEDULE_EDGE_OLD_CONFLICT_NEW);
 			}
 		}
 	}
+	if (a_old != NULL && b_old != NULL &&
+	    pkg_jobs_schedule_direct_depends(a_old, b_old)) {
+		if (!(a_new != NULL && b_new != NULL &&
+		    pkg_jobs_schedule_direct_depends(a_new, b_new))) {
+			return (PKG_SCHEDULE_EDGE_OLD_DEP_OLD);
+		}
+	}
 
 	return (PKG_SCHEDULE_EDGE_NONE);
 }

tests/frontend/upgrade.sh

@@ -12,7 +12,8 @@ tests_init \
 	dir_is_symlink_to_a_dir \
 	vital \
 	vital_force \
-	vital_force_cant_remove
+	vital_force_cant_remove \
+	upgrade_with_dependency
 
 issue1881_body() {
 	atf_check -s exit:0 sh ${RESOURCEDIR}/test_subr.sh new_pkg pkg1 pkg_a 1
@@ -422,3 +423,89 @@ Your packages are up to date.
 	ERROR=""
 	atf_check -o inline:"${OUTPUT}" -e inline:"${ERROR}" -s exit:0 pkg -o REPOS_DIR="$TMPDIR/repoconf" -r ${TMPDIR}/target -o PKG_CACHEDIR="$TMPDIR" -o FORCE_CAN_REMOVE_VITAL=NO upgrade -fy myplop
 }
+
+upgrade_with_dependency_body() {
+	atf_check -s exit:0 sh ${RESOURCEDIR}/test_subr.sh new_pkg "testa" "testa" "1.0"
+	atf_check pkg create -M testa.ucl -o ./repo
+
+	atf_check -s exit:0 sh ${RESOURCEDIR}/test_subr.sh new_pkg "testb" "testb" "1.0"
+	cat << EOF >> testb.ucl
+deps: {
+	testa: {
+		origin: "testa",
+	}
+}
+EOF
+	atf_check pkg create -M testb.ucl -o ./repo
+
+	cat << EOF > pkg.conf
+PKG_DBDIR=${TMPDIR}
+REPOS_DIR=[]
+repositories: {
+	local: { url : file://${TMPDIR}/repo }
+}
+EOF
+	atf_check \
+		-o inline:"Creating repository in ./repo:  done\nPacking files for repository:  done\n" \
+		-e empty \
+		-s exit:0 \
+		pkg -C ./pkg.conf repo ./repo
+
+	atf_check \
+		-o ignore \
+		-s exit:0 \
+		pkg -C ./pkg.conf update -f
+
+	atf_check \
+		pkg -C ./pkg.conf install -qy testb
+
+	rm -r ./repo
+
+	atf_check -s exit:0 sh ${RESOURCEDIR}/test_subr.sh new_pkg "testa" "testa" "2.0"
+	atf_check pkg create -M testa.ucl -o ./repo
+
+	atf_check -s exit:0 sh ${RESOURCEDIR}/test_subr.sh new_pkg "testb" "testb" "2.0"
+	cat << EOF >> testb.ucl
+deps: {
+	testa: {
+		origin: "testa",
+	}
+}
+EOF
+	atf_check pkg create -M testb.ucl -o ./repo
+
+	atf_check \
+		-o inline:"Creating repository in ./repo:  done\nPacking files for repository:  done\n" \
+		-e empty \
+		-s exit:0 \
+		pkg -C ./pkg.conf repo ./repo
+
+	atf_check \
+		-o ignore \
+		-e empty \
+		-s exit:0 \
+		pkg -C ./pkg.conf update -f
+
+OUTPUT="Updating local repository catalogue...
+local repository is up to date.
+All repositories are up to date.
+Checking for upgrades (2 candidates):  done
+Processing candidates (2 candidates):  done
+Checking integrity... done (0 conflicting)
+The following 2 package(s) will be affected (of 0 checked):
+
+Installed packages to be UPGRADED:
+	testa: 1.0 -> 2.0
+	testb: 1.0 -> 2.0
+
+Number of packages to be upgraded: 2
+${JAILED}[1/2] Upgrading testa from 1.0 to 2.0...
+${JAILED}[2/2] Upgrading testb from 1.0 to 2.0...
+"
+
+	atf_check \
+		-o inline:"${OUTPUT}" \
+		-e empty \
+		-s exit:0 \
+		pkg -C ./pkg.conf upgrade -y
+}