add successcache support to mpbb #1

jmroot · 2016-11-17T15:17:02Z

@macports/infrastructure please test, I don't have a local buildbot running.

https://trac.macports.org/ticket/52765

Check if the archive for the requested port exists in its installed location, and if so, simply skip installing. Do the same for each dependency. Dependencies are still built if not already installed (even if the requested port is somehow already installed), so their archives can be uploaded. Deactivate all ports between builds when installing dependencies. Activate all the dependencies after they are all installed iff the requested port will need to be built. Fixes: https://trac.macports.org/ticket/52765

mojca · 2016-11-17T15:30:41Z

Wonderful, thanks a lot. I have one request though. Could you perhaps try to move the code to mpbb-list-subports? My reasoning behind this is that a trivial commit touching a few thousand ports with pre-existing packages will actually start a few thousand individual build jobs that won't do anything substatial, but will happily "clog" the infrastructure. If we implement the logic already in the job that decides which ports to build, all those jobs will be skipped.

The only minor difference (disadvantage) in that approach could be that an existing package that has accidentally not been uploaded before will not be uploaded either (let's say that the commit fixes the licence which makes the package distributable), but I would try to address that differently. We could have a job that syncs the packages once per week or so.

mojca · 2016-11-17T15:36:22Z

mpbb-install-dependencies

-        else
+        # $option_prefix and $thisdir are set in mpbb
+        # shellcheck disable=SC2154
+        if [[ -f $("${option_prefix}/bin/port-tclsh" "${thisdir}/tools/archive-path.tcl" "${depname}" "${depvariants}") ]]; then


This part looks a little bit weird to me. In my opinion we should not even run install-dependencies when the package exists. On the other hand, when we actually need dependencies for a new port, this step will skip the required activation and most likely postpone it to the "install" step of the port itself, which is where we don't want the activation logs of dependencies.

If anything, you should probably test for existence of the port rather than for existence of dependencies. And then skip the whole chunk of installing dependencies rather that counting existing ones.

The activation is not skipped, it's just done all in one command at the end. Deactivating between builds is important for reasons unrelated to success cache.

If we don't assume anything about dependencies having previously built successfully then this part is necessary. If we assumed everything worked ideally, the install-dependencies step would be entirely unnecessary except to check the failcache and do the actual activation.

install-dependencies is super useful for many reasons:

it doesn't clutter the logs of port installation (one can concentrate on real problems with port itself rather than browsing through zillion lines of dependencies being built)

when we set up a new build slave, we don't have any dependencies yet, so some dependencies actually need to be built, not just activated

But, in the ideal case, the builds would be scheduled in dependency order, and we would build 'all' before doing anything else. Obviously everything does not currently work ideally. :)

Even then you can have the case of A that depends on B that depends on C. C is broken and gets fixed at some point, we forget to rebuild B, but A gets upgraded and then builds B on the go.

I'm sure we can find plenty other cases where building dependencies will be required.

Dependency order should be fixed at some point, but I'm not sure if we can always afford to build all packages. For one, I'm not sure if PPC would ever catch up if we start building all packages on it. I find it way more useful to keep building the latest commits and some hand-picked packages on the build slave for a while after it gets added. OK, I guess that's enough nit-picking on dependencies :)

OK, sure. All the more reason why we should upload B's archive when we can.

mojca · 2016-11-17T15:39:51Z

mpbb-install-port

+    if [[ -f $("${option_prefix}/bin/port-tclsh" "${thisdir}/tools/archive-path.tcl" "$@") ]]; then
+        echo "$@ already installed, nothing to do"
+        # log: summary for the portwatcher
+        echo "Building '$port' ... [OK]" >> "$log_subports_progress"


I would probably prefer ... [SKIP] to [OK]. But as mentioned earlier, I would put this to the part that decides which dependencies to build. (The code could potentially remain here as a safeguard anyway.)

Does the portwatcher know about [SKIP]?

Probably not yet, but this is easy to add.

raimue · 2016-11-17T15:49:44Z

I am not convinced it is a good idea to depend on the existence of produced archives. That means we have to keep all binary packages on the buildslaves, which might have limited disk space. It will be difficult to find out which old archives can be pruned.

We already have the failcache in place that has a callback for successful builds. Wouldn't it be better to store a success flag at this point, indexed by the canonical port name/epoch/version/revision/variants?

Another thing to consider, will there ever be a need to force a build ignoring the success cache?

mojca · 2016-11-17T16:07:41Z

If limited disk space is an issue for us, then we cannot even afford not to cancel the build job for something like macports/macports-ports#43. (Not that I believe the build would succeed anyway.)

I had an impression that we were deleting old versions of packages after successful build of the new version, but maybe I'm wrong and we are only deleting old distfiles.

My initial idea was to check for existence of the binary archive as well, but then again, a success flag similar to failcache might to be such a bad idea.

Another thing to consider, will there ever be a need to force a build ignoring the success cache?

I would rather ask: will we ever want to actually force a rebuild of a package? What about replace the existing binary archive? In our current implementation, even if you would "force rebuild" (ignoring success cache), all that would happen would be activation and de-activating of an existing archive. I can imagine that we would sometimes want to upgrade Xcode and try to rebuild a port with a newer compiler, or check again which warnings the compilation was throwing ... Not sure.

jmroot · 2016-11-17T16:16:04Z

Yes, non-current port versions are uninstalled by the cleanup step.

Can we save worrying about the effect on a system for selectively uninstalling some current port versions from the buildslaves for when that is implemented?

If we're going to start rebuilding packages then we need package revisions.

raimue · 2016-11-17T16:32:00Z

Okay, if the non-current versions are already uninstalled, then this should be fine.

If I remember correctly, on the old mpab-buildbot we were rebuilding when the port name was explicitly specified in a forced build, but we did not distribute any package produced that already existed. I don't know if there is a use case for this, so unless we have one we should ignore this for now.

jmroot · 2016-11-17T16:37:02Z

I don't think it ever rebuilt the same version. It may have run 'port install ' on a forced build which would have just activated.

larryv · 2016-11-17T17:44:18Z

@mojca re: list-subports

We should be able to reuse archive-path.tcl there if we want to.

mojca · 2016-11-17T18:00:02Z

Actually I wouldn't mind if what you implemented in archive-path.tcl was part of MacPorts base.

In any case I would probably not try to reuse it there, but use the "if binary already exists" (either based on your archive-path or based on some success cache database) in list-subports exclusively. In my opinion it still makes a lot more sense to completely skip building existing ports than to start a zillion of new jobs on the portbuilder.

larryv

LGTM, although I have not tested it.

larryv · 2016-11-17T17:51:18Z

tools/archive-path.tcl

+    set variations ""
+}
+
+if {[catch {set one_result [mportlookup $portname]}] || [llength $one_result] < 2} {


I think this can be

[catch {[mportlookup $portname]} one_result]

larryv · 2016-11-17T17:52:21Z

tools/archive-path.tcl

+
+array set portinfo [lindex $one_result 1]
+if {[info exists portinfo(porturl)]} {
+    if {[catch {set mport [mportopen $portinfo(porturl) [list subport $portinfo(name)] $variations]}]} {


I think this can be

[catch {[mportopen $portinfo(porturl) [list subport $portinfo(name)] $variations]} mport]

larryv · 2016-11-17T17:57:30Z

tools/archive-path.tcl

+if {[llength $::argv] > 1} {
+    set variations [split_variants [lindex $::argv 1]]
+} else {
+    set variations ""


lindex returns an empty value if the index is too large, so I think we could avoid the conditional altogether, right?

set variations [split_variants [lindex $::argv 1]]

jmroot · 2016-11-17T18:23:42Z

archive-path.tcl was just copied from MPAB's archivepath.tcl. You're probably right on the style points.

raimue · 2016-11-17T18:27:35Z

Actually I wouldn't mind if what you implemented in archive-path.tcl was part of MacPorts base.

port -q location <port> [@<version+variants>], although it will only work for installed ports.

larryv · 2016-11-17T18:32:31Z

@jmroot

archive-path.tcl was just copied from MPAB's archivepath.tcl. You're probably right on the style points.

Just wanted to check if I was overlooking something. If they're actually equivalent, feel free to let it go, and I'll do spring cleaning one day when I'm bored. 😝

jmroot · 2016-11-20T06:50:39Z

In any case I would probably not try to reuse it there, but use the "if binary already exists" (either based on your archive-path or based on some success cache database) in list-subports exclusively.

This would require list-subports to become aware of variants.

jmroot · 2016-11-21T09:52:03Z

Updated with a simple non-variant-aware exclusion check in list-subports.

jmroot · 2016-11-25T12:57:26Z

@mojca Are you satisfied with this change?

mojca · 2016-11-25T15:06:37Z

You may ignore my request for changes anyway, it's not like I'm the authority here :).

I'm happy about modification you made in mpbb-list-subports that now skips building existing binaries altogether. What I don't quite understand is:

Why we still need the second layer of testing for existence of the package before installing the port and its dependencies (if that port shouldn't make it to the port builder anyway)
What's the point of postponing activation of some dependencies towards the end? Now dependencies that haven't been built previously would be installed first (and will also activate some other dependencies that you intentionally skipped as part of the process), pre-existing dependencies would be skipped and then everything missing would be activated at the end, some dependencies that you'll try to activate will already be active by then, counter of installed dependencies will be broken (not that this is a serious issue). I'm not completely sure if "${option_prefix}/bin/port" -d activate ${dependencies} will work as expected (it might, but I don't know how variants are handled).

I didn't test the changes yet.

It doesn't have to be implemented now, but I would proposed to add support for a force flag to list-subports that would ignore both "success cache" and "fail cache", so that one could in principle force activation of all dependencies and the port itself as well as try to rebuild dependencies. (We can add a checkbox to the user interface later.)

Use cases:

special cases like the one with wine that Ryan mentioned;
it could happen that the package exists locally, but not on the server:
- one of dependencies could have its licence fixed or could remove dependency on a package with a conflicting licence: that would make the package distributable;
- maybe there were some spurious leftovers on the system that resulted in an error during activation of the package – then the package would not be uploaded;

jmroot · 2016-11-25T15:44:39Z

Why we still need the second layer of testing for existence of the package before installing the port and its dependencies (if that port shouldn't make it to the port builder anyway)

Some preliminary support for the force flag you mentioned. I thought it would be confusing if you forced a build and then it just did nothing because the subport list ended up being empty, but it seems wasteful to actually run the build. A log that says it's already built seemed like a good compromise.

What's the point of postponing activation of some dependencies towards the end?

We deactivate all ports between builds so that only each port's declared dependencies are active when it is built. This is just correcting the behaviour of mpbb-install-dependencies in this regard. The success cache makes this procedure less expensive.

I thought the counter was more important for (relatively lengthy) builds than for (relatively quick) activations. I guess there could be a second counter for activations if needed.

jmroot · 2016-11-27T21:10:40Z

Tested, seems to be working fine. Needs one change to master.cfg deployed first, but otherwise I'd consider this ready to merge. Please feel free to tweak afterwards.

mojca requested changes Nov 17, 2016

View reviewed changes

jmroot assigned jmroot and unassigned jmroot Nov 17, 2016

larryv added the enhancement label Nov 17, 2016

larryv reviewed Nov 17, 2016

View reviewed changes

exclude already-built ports in list-subports step

d1b390e

jmroot merged commit 62834a4 into macports:master Nov 29, 2016

jmroot deleted the successcache branch November 29, 2016 23:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add successcache support to mpbb #1

add successcache support to mpbb #1

jmroot commented Nov 17, 2016

mojca commented Nov 17, 2016

mojca Nov 17, 2016

jmroot Nov 17, 2016 •

edited

Loading

mojca Nov 17, 2016

jmroot Nov 17, 2016

mojca Nov 17, 2016

jmroot Nov 17, 2016

mojca Nov 17, 2016

jmroot Nov 17, 2016

mojca Nov 17, 2016

raimue commented Nov 17, 2016

mojca commented Nov 17, 2016

jmroot commented Nov 17, 2016

raimue commented Nov 17, 2016

jmroot commented Nov 17, 2016

larryv commented Nov 17, 2016

mojca commented Nov 17, 2016

larryv left a comment

larryv Nov 17, 2016

larryv Nov 17, 2016

larryv Nov 17, 2016

jmroot commented Nov 17, 2016

raimue commented Nov 17, 2016

larryv commented Nov 17, 2016

jmroot commented Nov 20, 2016

jmroot commented Nov 21, 2016

jmroot commented Nov 25, 2016

mojca commented Nov 25, 2016

jmroot commented Nov 25, 2016

jmroot commented Nov 27, 2016

add successcache support to mpbb #1

add successcache support to mpbb #1

Conversation

jmroot commented Nov 17, 2016

mojca commented Nov 17, 2016

Choose a reason for hiding this comment

jmroot Nov 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raimue commented Nov 17, 2016

mojca commented Nov 17, 2016

jmroot commented Nov 17, 2016

raimue commented Nov 17, 2016

jmroot commented Nov 17, 2016

larryv commented Nov 17, 2016

mojca commented Nov 17, 2016

larryv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmroot commented Nov 17, 2016

raimue commented Nov 17, 2016

larryv commented Nov 17, 2016

jmroot commented Nov 20, 2016

jmroot commented Nov 21, 2016

jmroot commented Nov 25, 2016

mojca commented Nov 25, 2016

jmroot commented Nov 25, 2016

jmroot commented Nov 27, 2016

jmroot Nov 17, 2016 •

edited

Loading