Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-8728

SCons pybind11 builds re-link unnecessarily

        Details

        • Type: Bug
        • Status: Done
        • Priority: Major
        • Resolution: Done
        • Fix Version/s: None
        • Component/s: None
        • Labels:
          None
        • Templates:
        • Story Points:
          6
        • Epic Link:
        • Team:
          Data Release Production

          Description

          When rebuilding the pybind11 branch of afw, I noticed that scons was frequently rebuilding products even when no changes had occurred. Knowing that this used to be a problem in Swig builds based on whether SCons processed Swig files before or after .cc files, I tried explicitly listing the build targets, as this was the workaround for the Swig problem. This did prevent an immediate build, but revealed another way to trigger the problem that may be easier to debug:

          If you do:

          scons lib python

          and then

          scons lib python tests

          You'll note that it re-links all of the pybind11 modules before running all of the tests. If you run scons with --debug=explain, it will report that the dependencies of libafw.so have changed, and hence that file needs to be re-linked. It then rebuilds all of the pybind11 modules, since these each link against libafw.so. I don't understand why it thinks the dependencies of libafw.so change when we add tests to the target list (they may also change under other weird circumstances), but that's what we should fix.

          Giving this a lot of SPs since SCons dependency logic debugging can be hard.

            Issue Links

            blocks

            Story - Created by JIRA Agile - do not edit or delete. Issue type for a user story. DM-9100 Merge pybind11 branches to master

            • Major - Major loss of function.
            • Done

              Activity

              Ascending order - Click to sort in descending order
              Hide
              Permalink
              jbosch Jim Bosch added a comment -

              Fun discoveries, which seem like they have to be SCons bugs of some kind:

              • libafw.so is rebuilt if you add or remove "tests" from the list of targets, because doing so changes the order of its dependencies (a list of .os files). This happens despite the fact that immediately before we call SharedLibrary with a list of sources (.cc files), we sort that list. We might be able to work around this by manually generating the list of .os files with SharedObject and then sorting those and passing them to SharedLibrary, but that may just be wishful thinking.
              • When libafw.so is rebuilt, all of the pybind11 modules that link to it are re-linked, because SCons claims that it has changed. However, we've told SCons to use md5 sums to decide when something has changed, and the md5 sum of libafw.so hasn't changed. I haven't yet been able to find any way to ask SCons whether it understands that it's supposed to use md5 sums, or found out where it's saving those hashes.
              Show
              jbosch Jim Bosch added a comment - Fun discoveries, which seem like they have to be SCons bugs of some kind: libafw.so is rebuilt if you add or remove "tests" from the list of targets, because doing so changes the order of its dependencies (a list of .os files). This happens despite the fact that immediately before we call SharedLibrary with a list of sources (.cc files), we sort that list. We might be able to work around this by manually generating the list of .os files with SharedObject and then sorting those and passing them to SharedLibrary , but that may just be wishful thinking. When libafw.so is rebuilt, all of the pybind11 modules that link to it are re-linked, because SCons claims that it has changed. However, we've told SCons to use md5 sums to decide when something has changed, and the md5 sum of libafw.so hasn't changed. I haven't yet been able to find any way to ask SCons whether it understands that it's supposed to use md5 sums, or found out where it's saving those hashes.
              Hide
              Permalink
              jbosch Jim Bosch added a comment -

              Not promising I'm going to be the one to finish this, but I'm definitely working on it (mostly waiting for test runs to finish building).

              Show
              jbosch Jim Bosch added a comment - Not promising I'm going to be the one to finish this, but I'm definitely working on it (mostly waiting for test runs to finish building).
              Hide
              Permalink
              jbosch Jim Bosch added a comment -

              I believe this is ready for review. Tim Jenness, could you take a look?

              I've only fixed one of the problems I noted above - the fact that the lib target spuriously rebuilt when the top-level build targets change. That was due to dependency ordering changes, which was in turn due to sorting of those dependencing not working. With that change, the second problem seems much harder to trigger (possibly impossible to trigger accidentally), so I'm much less concerned about it. I'd love to fix that too, but I have no idea how to debug it, let alone fix it.

              Note that while this is part of the pybind11 epic since it's a much more obvious problem on the pybind11 branches, the problem is not limited to the pybind11 branch and hence I've made my changes directly on master and intend to merge to master when complete; we can then rebase the pybind11 branches on that.

              In addition to the changes in sconsUtils, I've also replaced the afw/lib/SConscript file with the standard boilerplate; it seems to have just been reimplementation of the relevant part of BasicSConscript.lib(), rather than a meaningful difference. GitHub PRs will be up shortly, and Jenkins is in progress.

              Show
              jbosch Jim Bosch added a comment - I believe this is ready for review. Tim Jenness , could you take a look? I've only fixed one of the problems I noted above - the fact that the lib target spuriously rebuilt when the top-level build targets change. That was due to dependency ordering changes, which was in turn due to sorting of those dependencing not working. With that change, the second problem seems much harder to trigger (possibly impossible to trigger accidentally), so I'm much less concerned about it. I'd love to fix that too, but I have no idea how to debug it, let alone fix it. Note that while this is part of the pybind11 epic since it's a much more obvious problem on the pybind11 branches, the problem is not limited to the pybind11 branch and hence I've made my changes directly on master and intend to merge to master when complete; we can then rebase the pybind11 branches on that. In addition to the changes in sconsUtils, I've also replaced the afw/lib/SConscript file with the standard boilerplate; it seems to have just been reimplementation of the relevant part of BasicSConscript.lib() , rather than a meaningful difference. GitHub PRs will be up shortly, and Jenkins is in progress.
              Hide
              Permalink
              tjenness Tim Jenness added a comment -

              Looks okay to me.

              Show
              tjenness Tim Jenness added a comment - Looks okay to me.
              Hide
              Permalink
              jbosch Jim Bosch added a comment -

              Merged to master.

              Show
              jbosch Jim Bosch added a comment - Merged to master.

                People

                • Assignee:
                  jbosch Jim Bosch
                  Reporter:
                  jbosch Jim Bosch
                  Reviewers:
                  Tim Jenness
                  Watchers:
                  Jim Bosch, Pim Schellart, Tim Jenness
                • Votes:
                  0 Vote for this issue
                  Watchers:
                  3 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:

                    Development